Styleguide and theme code de-duplication #1651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
March 31, 2018 12:08
…mpiler into style_uni
does not update base scss files for colors
CSS change from master 18cbaab
…light button color
|
Added an Up For Grabs demo for this PR here: |
|
@bradymiller thanks for taking a look!
Nothing I can tell... But I don't know the system like y'all do. If I had a "golden path" to click through, I could check against an existing demo.
Just for developers as long as we're committing the CSS. In the future, i'd prefer if the CSS wasn't committed so devs only edited the SCSS. If we did that, we'd have to add a "build step" to our deploy and release strategies. I don't think we're there yet. In the meantime, @robertdown and I were thinking that we could just minify all the theme CSS for now (e.g. run |
|
hi,
-brady |
bradymiller
reviewed
May 21, 2018
interface/themes/rtl.css
Outdated
|
|
||
| .select2-search, .select2-results{ | ||
| direction: rtl !important; | ||
| } |
There was a problem hiding this comment.
Just checking if the above 4 lines were missed (as I recall, this was recently added to codebase)
|
Hi @d3sandoval The demo looks great. Thanks for using a colored stylesheet :). It is amazing what you have been able to achieve. On my quick perusal I did note a few things
What it should be
What it is
Similarly the infamous help modal What it should be
What it is
Tabs in Administration > Globals What it should be
What it is
I am certain that a minor tweak on your part will fix these :)) |
|
All easy fixes, so far! I'll address these in a follow up commit. |
|
Looking forward to reviewing all this, exciting stuff! I think now is actually a good time to introduce minification. We should have done it a long time ago. We can add a comment to the top of the mini field style saying it’s been auto generated and a link to some documentation. |
|
hi @robertdown , |
|
Considering there are many developers contributing to the codebase leaving it unminified but with adequate notification in the individual stylesheet indicating that it is a generated stylesheet would be helpful. |
|
Minification prevents accidental edits of that file, which is a bit of a brutish way of doing that, but it’s a great way to really indicate CSS files can no longer be edited directly. An unminified CSS file is asking to be edited whereas a minified one scares people away (which is what we want in this case). There’s also the regular benefits of minification, speed enhancement, yada yada |
|
We should be treating CSS essentially as compiled code, a black box not to be touched. |
|
hi @robertdown , Note as folks learn this, the likely workflow will happen: I am also a bit distrustful of black boxes in the codebase; shouldn't this need to earn our trust :) (ie. after seeing how it works when folks make changes etc., which is impossible to follow with minified files)? |
|
You can follow changes, we'd generate a sourcemap for minified files. It's not really a black box, that was a bad example. If we don't minify now, we should set a timetable for when we will begin minifying, otherwise it'll get tossed in the back burner and forgotten about. I'd recommend 6 weeks from the date this PR is accepted. |
Closed
|
Pushed changes as requested by @bradymiller and @zbig01 . here's a video describing how that's possible without even spinning up an OpenEMR instance: https://drive.google.com/open?id=1nKXSVHFZhmZx0gNxsbpGQa-uof35dABC Will have a commit that adds a note to the top of the generated css in a few minutes |
|
Added the helper text at the top of the files to point to the README. If you're curious, here's what the style_light.css looks like minified: |
|
@bradymiller i think this is ready to merge, unless you'd like me to commit the minified css? |
|
@d3sandoval , ask and you shall receive :) |
|
@bradymiller great! Feel free to reach out if you need any help working with the new SCSS |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a massive PR but the goal is simple:
Create a consistent way to develop and use themes
This code introduces gulp as a build system for OpenEMR's core theme files and storybook as a documentation site. Files are now broken into their own SCSS files and share more code than ever before! More details on this can be found in the README.md in
interfaceand on the styleguide website: http://openemr-interface.surge.sh/Because all themes share the majority of their code now, the generated CSS has been rearranged quite a bit. This was done in order to take advantage of the cascading nature of CSS. In short, theme files import the
core.scss(shared code) and then provide their overrides later on in the file (either through individual css rules or through another .scss import).I was able to use a local docker container to test if I broke anything during my reorganization. I doubt I was able to catch everything so please do pull down the code and test it locally.
npm run devshould let you change the base .scss files and see updates as things get saved and generated. More info on that in the README.Once this is approved, additional work can be done to chop the differences between themes down even further. Most notably, font-sizes are declared in
%,em,pxandptacross multiple files. I've also left some todos in the README to note what other work hasn't been done from the original conversation: https://community.open-emr.org/t/user-interface-design-style-guide-the-beginnings/9634/24Feel free to ping me if you have any questions!
below added by bradymiller:
Up For Grabs demo for this PR is here: https://www.open-emr.org/wiki/index.php/Development_Demo#Nu_-_Up_For_Grabs_Demo