Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Added Ability to Customize CSS Styles for <table> HTML Elements #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SkyeHoefling
Copy link

@SkyeHoefling SkyeHoefling commented Jul 9, 2018

Added ability to inject custom CSS classes for the <table> HTML element.

Description of PR...

Added CSS Customization to the module to allow CSS classes to be injected into the <table> HTML element when the <table> is generated. It is super easy to add new css styles for a module to get things to look exactly how you want but if you use a css framework that relies on classes you will find yourself spending more time than you would like getting things to look just how you want.

This PR sets the building blocks for adding custom styles to particular elements in the Wiki Module. I found it unnecessarily difficult to add my bootstrap classes to the <table> element. I didn't want to create custom styles for tables I just wanted to re-use what Bootstrap gives me for free. In the Wiki Configuration you can now specify in plain text the classes you want injected

image

It even comes with a tooltip if this doesn't make sense to the user:
image

After configuring your Table Styles all of the tables start looking pretty:

Bootstrap Classes
image

No Classes
This is what it looked like before we added our custom css classes
image

Changes made

  • Added new CSS Styles section to Wiki Configuration
  • Added Table Styles to the new Configuration Section
  • Added new SQL Migration script to add necessary new TableStyles to the Wiki_Settings Table
  • Updated the WikiModuleBase to add custom styles if they exist where it generates the HTML <table>

PRChecklist

  • Feature solution
  • Update manifest file to version 05.01.00
  • Update sql provider migration script to match version 05.01.00.xx

Close #10

…tion which allows us to specify custom css classes to be injected for each table element that is created.
@SkyeHoefling SkyeHoefling changed the title Added Ability to Customize CSS Styles for <table> HTML Elements [WIP] Added Ability to Customize CSS Styles for <table> HTML Elements Jul 9, 2018
@SkyeHoefling
Copy link
Author

Can someone please advise on what version I should update the manifest file and the sql migration scripts to?

@valadas
Copy link
Member

valadas commented Jul 10, 2018

I don't think we should update the module version in pull requests, I think that should be done only by the persons merging pull requests. @DNNCommunity/module-dev-team any opinions on this? I can make it a subject for next meeting if we want to discuss more on the subject.

@SkyeHoefling
Copy link
Author

@valadas I 100% agree with you. The problem we run into is in the case here we had to specify a SQL Migration Script and it appears that the naming convention in Dnn has been we match the provider name with the version it is associated with. I am not sure if that is the case or not but it certainly looks like, the provider scripts should just be DB Migration scripts that increment on their own independently from the version of the module

In my experience the best practice would be to set up a Build for each community module and then the build could manage the version number instead of having us check it in to source control.

@valadas
Copy link
Member

valadas commented Jul 10, 2018

Yeah I get what you mean, what if each time we do a release, after we immediately bump the build number by 1 for the next stabilization release and create empty db script for the next minor and major release. The people submitting pull requests can add to the existing file... Just and idea, I think we should discuss that so that everybody is on the same page for all modules.

@SkyeHoefling
Copy link
Author

I think that could be a viable approach to this problem. The only issue I see with it is new devs testing SQL changes locally. Personally I prefer to follow the extension installation process instead of side-loading modules. If I am making several changes in the SQL Provider file this could get mess up my development cycle.

If we go with this approach the convention NEEDS to be documented in each community module wiki as part of the contribution guidelines. The last thing any community member would want is putting in hard work to find out they didn't follow the guidelines.

@david-poindexter
Copy link
Contributor

Hey @valadas and @ahoefling - this is indeed a unique challenge. Historically speaking, it would be the discretion of the project owner to determine the version number and any PR would need to be coordinated between the contributor and project owner. So in this instance where the PR requires a SQL upgrade script as a part of the next module release would need to be coordinated. One approach is to split the PR into smaller, more manageable chunks and reference them. Otherwise, keep the commits in the same PR and just coordinate the version numbers with the project owner. The benefit here is that the project owner agrees to the release schedule and is committing to merging said PR into the next release with the correct version number. This has a trickle effect of course, because the manifest also needs to be updated accordingly.

All that said, a build process could work too. @ahoefling any guidance/recommendations/contributions you have on that end would ideally need to be brought to the core module team so consistency would be insured across all core, and ultimately other DNN Community modules as well.

@EPTamminga
Copy link
Member

@valadas @ahoefling @nvisionative
Since DNN8 you can have multiple (incremental) SQL scripts in one release. E.g. if you are working on release 08.01.01, you can have SQL scripts 08.01.01.xx, where the xx can be a number from 00 to 99.
They should all be included in the final release, or you can opt to merge all de incremental scripts into one final release script.

Does this solve the issue here at hand?

@david-poindexter
Copy link
Contributor

Ah, that is great to know @EPTamminga - I was unaware of that "feature". That solves the SQL script version number issue. However, that does not solve the manifest file issue.

@valadas
Copy link
Member

valadas commented Jul 10, 2018

@EPTamminga interesting,i did not know about that. So in the manifest you would just have the 08.01.01 script defined and it would run all the 08.01.01.xx scripts?

@SkyeHoefling
Copy link
Author

That actually simplifies the problem greatly. I can update the Pull Request to follow this format.

My next big question is what numbering should I use for the provider? I arbitrarily picked 05.01.0X should we keep that so I would update the provider to 05.01.01.xx or should we update it to something like 09.02.00.xx

@EPTamminga
Copy link
Member

I have not tried it myself, but this is what is noted in the release notes of dnn8:

Incremental upgrade support now possible via SqlDataProvider, cleanup files and configuration merge files. This allows changes to the platform product or third party extensions to be made without requiring a new versioned release.

So I think the n.m.i.xx scripts are run after the n.m.i release sql script and you can stay on the n.m.i version.

@EPTamminga
Copy link
Member

The manifest version # update now can be done on a full new release

@SkyeHoefling
Copy link
Author

Sounds like we have a direction of using the incremental provider number, which is great! Thanks @EPTamminga for the information on the incremental provider numbers

We still need to decide if we want our provider number to match the version number of the module. Does anyone have thoughts on this?

@valadas
Copy link
Member

valadas commented Jul 11, 2018

I think we should, it helps to know what belong to which module version. At least all the modules I have seen follow that rule.

@valadas
Copy link
Member

valadas commented Jul 11, 2018

The last published version is 5.0.1 and this module currently does not have a dedicated maintainer, so in my opinion we should go for 5.1.0 since this would technically be a new feature, if it was a bug fix, I would have went for 5.0.2.

@valadas
Copy link
Member

valadas commented Jul 11, 2018

That being said, I see no problem for this pull request to stay at the version you put in (5.0.7) since it does not conflict anything. @EPTamminga are you ok with mergin this as is? The script would still run if the next release is 5.1.0

@EPTamminga
Copy link
Member

Since there is no hard need for an immediate release with this feature, I would go for a 5.1.0 release and make that available 1 aug 2018?
That being said, are we, for core modules (like they decided for DNNPlatform), also going for a RC to be available for 2 weeks, before making the final release of a module?

@SkyeHoefling
Copy link
Author

SkyeHoefling commented Jul 11, 2018

I will update todo items at the top of this to target 05.01.00.xx for the provider

@EPTamminga
Copy link
Member

We can also use the "Project" tab of the repo for ToDo/roadmap sort of things.
And/or we can create Milestones to attach to issues to get things organized

@david-poindexter
Copy link
Contributor

@EPTamminga I would vote no on doing release candidates for core modules.

@valadas
Copy link
Member

valadas commented Jul 11, 2018

I have put this as a subject for our next meeting, we need to coordinate some kind of roadmap and do the same on all projects...

@SkyeHoefling
Copy link
Author

Does the release cadence block this PR? (I have updated tasks that still need to be done)
Is this a change that we should accept or does it add unnecessary bloat to the module?

@valadas
Copy link
Member

valadas commented Jul 11, 2018

I think when we know there is a higher risk (such as when it is a complete code conversion from vb to c#, a major architecture change, etc. We whould just mark that release as a major number such as X.0.0 and mark it as a pre-release with a warning it is not recommended for production...

@valadas
Copy link
Member

valadas commented Jul 11, 2018

@ahoefling no, I dont think in my opinion that anything blocks this PR. It is a small change that does not add bload and makes the module look much nicer to look at :) The only thing is that this module currently has no decicated maintainer at the moment. I am focusing right now on getting all modules Dnn 9.2 compatible, then prioritizing bugs and then I will go around through the modules that have no dedicated maintainers, so it will take a couple days/weeks before I get to this one.

@valadas valadas added this to the 5.0.2 milestone Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants