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

Update to newest Bootstrap 4 #2807

Open
tywrenn opened this issue Nov 20, 2019 · 139 comments
Open

Update to newest Bootstrap 4 #2807

tywrenn opened this issue Nov 20, 2019 · 139 comments

Comments

@tywrenn
Copy link
Contributor

@tywrenn tywrenn commented Nov 20, 2019

This has been pushed back for 4 years (as the latest revision we are on is from 2015) and it has now come to the time to address it - we should now begin updating to Bootstrap 4. This will take awhile as some frameworks may break during this transition to Bootstrap 4.

New things expected with Bootstrap 4:

  • Bootstrap 4 uses flexboxes instead, making content spacing act different than Bootstrap 3
  • Glyphicons are no longer implemented in Bootstrap and should be replaced with the FontAwesome framework as recommended by Bootstrap (FontAwesome can be installed here)
  • Navbars are now using flexbox as well and may behave differently
  • Tons of new components were added to Bootstrap 4, including some great utility CSS classes like w-50 to turn the width to 50%
  • Bootstrap 4 no longer supports IE8 or IE9 with this transition (yet it's best to transition away from Internet Explorer anyways)
  • Offsetting columns were renamed. e.g. Bootstrap 3 says col-md-offset-4 while Bootstrap 4 says offset-md-4
    For some more differences between the two frameworks, check out this article.
    If anyone is having any issues with any Bootstrap code contact me.

This issue is being actively sponsored by PEER Technologies PLLC.

To Do List:

  • Replace Old Code: Replaced pull-left and pull-right with new float-* class
  • Replace Old Code: Removed outdated col-xs-* and replaced with new col-* class
  • Replace Old Code: Removed outdated col-*-offset-* class format with new offset-*-* class format
  • Replace Old Code: Replaced table-condensed class with table-sm
  • Replace Old Code: Replacedbtn-xs class with btn-sm
  • Replaced the well class with the jumbotron class as wells aren't supported anymore (This can also be fluid with jumbotron-fluid)
  • Signer API fixed from migrating to Bootstrap 4
  • Redesigned OpenEMR themes to work around Bootstrap 4 (using docs on themes)
  • Replaced most theme colors with Bootstrap colors
  • RTL works fine
  • Reformatted CSS and JS files from /interface/modules/zend_modules/public

Things I will need help from the OpenEMR community in:

  • Replace outdated inline CSS styling with new Bootstrap variables
  • Help with fixing nav-pills as most of these navigations are using very outdated code that might break if changed
  • Help with replacing visible-* classes as they aren't supported anymore
  • Help sorting out CSS and SCSS conflicts while merging things into the core CSS
  • Angular dependency BS4 update.
  • Other code fixes.

Things that will be in a separate pull request in the future:

  • Moving MedEx CSS into core themes
  • Removing outdated CSS and JS files from /interface/modules/zend_modules/public
  • Adding more themes

To test out the new features coming from this pull request, a demo is available on the OpenEMR website: https://www.open-emr.org/wiki/index.php/Development_Demo#Gamma_-_Up_For_Grabs_Demo

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Nov 20, 2019

There is Bootstrap RTL yet it should be temporary until the full transition occurs. @mdsupport might also have some suggestions.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 20, 2019

First item to be addressed is to figure out where bs4 is currently being used so we can ensure we don't break anything bringing in latest bs4 release.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 20, 2019

@zbig01 You may have some comments on this matter.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 20, 2019

We don't current have BS4 in our dependency tree so checking is mute. I'm going to modify header class to allow BS4 include while ensuring BS3 is not included at same time.
This way we'll be ready for the carefully thought through leap.

@bradymiller

This comment has been minimized.

Copy link
Member

@bradymiller bradymiller commented Nov 20, 2019

You know you just want to drop BS3 and put BS4 in it's place to see what happens :)
Maybe we'll get lucky.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 20, 2019

lol, you know luck has not been my friend lately! I will however give it a try on my test server.

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Nov 20, 2019

I just tested 4.3.1 with RTL 4.3.1-6 on my Virtual Machine and it runs fine, you also have to have popper.js installed as well.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 20, 2019

Okay, i'll get the dependencies squared away in header class. Tonights dev demo reset will include.

@mdsupport

This comment has been minimized.

Copy link
Contributor

@mdsupport mdsupport commented Nov 20, 2019

you also have to have popper.js installed as well.

We have been using standard bootstrap.bundle.min.js which include Popper but not jQuery.

@mdsupport

This comment has been minimized.

Copy link
Contributor

@mdsupport mdsupport commented Nov 21, 2019

The Migration page of BS4 documentation is a must read for anyone considering BS4. The differences between BS3 and BS4 are sometimes subtle - e.g. .col-xs-6, is now .col-6.

Based on our expectation of testing efforts involved, it may be prudent to let BS3 and BS4 be selectable - maybe add bootstrap4 as the header component explicitly invoked by migrated pages.

@bradymiller

This comment has been minimized.

Copy link
Member

@bradymiller bradymiller commented Nov 21, 2019

It would be interesting to see what happens on a full conversion (we could set up a Up for Grabs demo to a PR that does this) to give us an overall take on how much things break. If we are lucky, we may get away with this (we've got several months until next release to clean up any bugs). Otherwise we may get stuck in the position where we are indefinitely trying to convert/test all scripts (we may need to do this, but would nice if we didn't need to do this).

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 21, 2019

Menus and custom menus have to be touched. Font-sizes and padding seem different and more of course.

  • maybe add bootstrap4 as the header component explicitly invoked by migrated pages.

Was my initial thought. Problem I see here is we'd need tandem theme and css files until done. However, that's doable where after all scripts are bootstrap 4 we simply delete old BS3 css files.

It would be interesting to see what happens on a full conversion

This would work if we got all hands on deck and freeze development till done.

I think before we move to BS4 we need to decide what we gain in doing so. I'm not experienced enough with BS4 to have an opinion accept it is active in development, I believe.
I was hoping @mdsupport who has been doing BS4 in their version for awhile now could lend some wisdom on this point.

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Nov 21, 2019

I think before we move to BS4 we need to decide what we gain in doing so. I'm not experienced enough with BS4 to have an opinion accept it is active in development, I believe.
I was hoping @mdsupport who has been doing BS4 in their version for awhile now could lend some wisdom on this point.

@sjpadgett I've been coding in Bootstrap 4 for awhile so I think I can help with that

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Nov 21, 2019

Also i've just tested Bootstrap 4 without any RTL on it and it seems stable and just needs a few fixes here and there

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 21, 2019

Okay @tywrenn ,
Why don't you go ahead and get your openemr branch current then open a PR. Call it something like refactor to BS4.
I'll then push the modified dependencies config to your branch. Afterwards @bradymiller can set up a Up for Grabs.
How do you want the default bootstrap to load. As is, bootstrap 3 where you may select Bootstrap4 in script header or default to bootstrap 4?
What do you think?

tywrenn added a commit to tywrenn/openemr that referenced this issue Nov 21, 2019
@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Nov 21, 2019

Since Bootstrap 4 runs fine without Bootstrap 3 (more testing needed though) we should just update to Bootstrap 4 and then start coding in all the code fixes which in my opinion shouldn't take too long.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 21, 2019

That's fine, if that's the way you want to attack.
Couple things though:

  • you can use the bundled version
  • must use UMD version for popper
  • may as well bring in popper-utils(unclear if bundle has)

This all is configured in our Header config at: openemr/config/config.yaml

did an edit if bundle is not use to auto load popper.

popper:
    basePath: %assets_static_relative%/popper.js/dist/umd/
    script: popper.min.js
    autoload: true
popper-utils:
    basePath: %assets_static_relative%/popper.js/dist/umd/
    script: popper-utils.min.js
    autoload: true
@mdsupport

This comment has been minimized.

Copy link
Contributor

@mdsupport mdsupport commented Nov 21, 2019

Maybe we'll get lucky.

If one is looking for status quo, good news is that current code with inline styles painfully controls the layouts. Add the overrides in emr stylesheets and existing code essentially insulates you from BS.
So one of the major decision for us was to position bs4 css last with aim to progressively eliminate emr's css as well as jQ UI...

For the project, suggestion would be to follow BS4 documentation on themes.

At the risk of repetition, we have focused efforts on small(ish) devices. That's why BS4 with an opinionated (their words) design philosophy has been very productive. Here is one such example :

Bootstrap only supports one modal window at a time. Nested modals aren’t supported as we believe them to be poor user experiences.

Glad the standard interface is moving to current ui component.

p.s.
In case anyone is interested, here is a POC approach that we tried and shows promise to get developers to focus on implementing business logic rather than formatting issues.

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Nov 21, 2019

@sjpadgett I'll be using the bundled version to make it easy for us. I'll add another commit to fix the config file - let me know if i'm missing anywhere else that needs changing besides the NPM and config file.

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Nov 21, 2019

Added to do list to the issue request to show our progress 👍

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 21, 2019

opinionated

Show me an engineer who isn't! Thanks for the info. Will be handy.

tywrenn added a commit to tywrenn/openemr that referenced this issue Nov 21, 2019
@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Nov 21, 2019

I was able to get around adding popper JS in this commit.

Here are some things I noticed immediately after upgrading:

  • I noticed immediately on upgrade the navbar doesn't really collapse too well when using Bootstrap 4.
  • Spacing fixes are DEFINITELY needed (probably cause of col offsets being changed)
  • Patient Portal needs a lot more fixes in Bootstrap 4
  • The login logo isn't centered anymore
@mdsupport

This comment has been minimized.

Copy link
Contributor

@mdsupport mdsupport commented Nov 21, 2019

Surely there are more elegant approaches but providing ours if it helps.

Since we did not want to touch current code until it was up for refactoring, we let jQ do the work. Here is an example of main frame code that is used by all tabs (just pass their own $):

// Utility functions
function utilSwitchClass(sel, add, rem) {
    sel.addClass(add).removeClass(rem);
}

// Common actions for all
function auto_common_transforms($) {
    utilSwitchClass($('.col-xs-12'), 'row col-12', 'col-xs-12');

    $('table').addClass('table table-sm table-hover table-striped table-borderless');
    $('thead').addClass('thead-light');

    $('.btn-group').addClass('btn-group-sm');

    $('input[type=button]').attr({'role':'button'}).addClass('btn');
    utilSwitchClass($('.css_button'), 'btn', 'css_button');
    $('.btn').addClass('btn-sm');

    $('.form-control').addClass('form-control-sm p-1');
}

Header library inserts a standard script that checks and calls a script specific formatter if it exists otherwise it calls auto_common_transforms. Here is a sample formatter for script /interface/patient/file/front_payment.php:

function auto_interface_patient_file_front_payment($) {
    auto_common_transforms($);
    $('.detail').removeClass('detail');
    $('form > div').addClass('form-group form-group-sm');
    $('form input, form select').addClass('form-control form-control-sm');
    $('tr').removeAttr('height');
    $('td label.radio-inline').parent().addClass('form-check form-check-inline');
    $('.radio-inline input').addClass('form-check-input pl-2');
    $('label.radio-inline').addClass('form-check-label p-2');
}

This strips some of inline styles and converts all forms to BS4 as a workaround. As seen from selectors, the php script is using a table for form rows that works well for probably 1024x768 desktop. Responsive design will need form field label linkages and probably a grid layout. Review, design and refactor efforts could be significant compared to few minutes needed for this function. Major saving is the testing time since php script is not affected at this stage.

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Nov 22, 2019

After using a linux sed replace method, I found that there were some 500+ lines of code that needed to be changed over to Bootstrap 4.

Most of the code I found was using col-xs which isn't supported, so replacing that with the new col tag will fix that area. Another big one was btn-xs, which isn't supported either and needs to be removed entirely due to their being no alternative.

@mdsupport I like this approach to fix this issue on a temporary scale, yet it should be something that we can easily toggle on and off so we can figure out where these changes are gonna be needed as we go along with this transition. Perhaps making a hidden toggle switch somewhere?

As far as Bootstrap RTL goes @sjpadgett i'd like to know what we currently use it for and if we should keep it or remove it as that's a bit of a gray area for me in terms of my knowledge. I would like us to use the Bootstrap theming standards as much as possible.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 22, 2019

As far as Bootstrap RTL goes

RTL is extremely important to OpenEMR. It is one of our keystones along with internationalization for languages.
We have hundred if not thousands of arabic and hebrew users. So, no RTL no BS4.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Nov 22, 2019

@matrix-amiel
Could you or one of your team take a look at the newer bootstrap 4 RTL and tell us how closely it fits with our current solution? We are trying to upgrade to bootstrap 4 in core code.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Dec 17, 2019

Yep, as you've said:) and I even pondered moving them to left but what I really want to do is redesign the page. Currently the mission is to get BS4 current. Title and header sizes will be addressed once we figure out what they will be.
I promise my friend, I will redesign that page, soon.

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Dec 17, 2019

@mdsupport @sjpadgett So to tackle loading Bootstrap into our SASS themes I tried loading it up. The thing I immediately noticed is it takes forever to build Bootstrap, especially if you put it in 3 SASS files. What are some ways that we can reduce this compile time? The main things i'll be changing in SASS with Bootstrap are the colors and it should be relatively easy SASS code.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Dec 17, 2019

@tywrenn Depends on what you hope to do. Are you planning to build theme on startup or all available themes on production build as now?
Also, Brady just redid and updated the build process dependencies(in working branch now) so are you using that?

@mdsupport

This comment has been minimized.

Copy link
Contributor

@mdsupport mdsupport commented Dec 18, 2019

we have been using bootstrap-magic for some time.

Sorry @tywrenn. We did not want to deal with complaints about colors, sizing and spacing. In any case, are you fully following their guidelines about themes?

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Dec 18, 2019

@mdsupport @sjpadgett I've been following the guidelines on themes to make sure that it all works. I'm actually just starting this so my first commit addressing this won't include things such as Bootstrap-Magic related SASS code. My approach is to get Bootstrap to load with the current themes and then start creating different ones. Colors in OpenEMR currently are all over the place and we'll need to sort them in order to allow themes to work.

And yes I'm up-to-date with the branch :)

tywrenn added a commit to tywrenn/openemr that referenced this issue Dec 18, 2019
- Bootstrap CSS is now implemented in the core.scss file to allow theming engine compatibility
- Replaces some colors with Bootstrap SASS variables to allow theming to work
- Adds comments on where things might need to be consolidated, moved, etc. for theming
- Changes login.css to a SASS file (this keeps being re-added as a CSS file in docker for some reason?) and adds it to the core CSS instead (this shouldn't be an issue)
- Provides a base core structure for Bootstrap theming to work (still needs work though)
- css_button classes and SASS-related files will be removed soon!
- Adds the NEW DARK THEME 😍 (This will be our first theme to test on to see if Bootstrap styling is working. So far it isn't working due to a lot of coloring issues.)
- Other bug fixes
tywrenn added a commit to tywrenn/openemr that referenced this issue Dec 19, 2019
@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Dec 19, 2019

Updated to-do list.

@bradymiller

This comment has been minimized.

Copy link
Member

@bradymiller bradymiller commented Dec 20, 2019

Angular is using most recent version on the bootstrap PR (all the js assets were updated to most recent versions on the PR); does this mean the "Angular dependency BS4 update." is completed?

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Dec 20, 2019

Whatever is the case with angular I don't think I used their BS but used portal theme. Leave this with me and i'll test soon.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Dec 21, 2019

This is crazy! We have tabNav class All over the place for the main purpose of circumventing bootstraps job.
Most places they're not really tabs but PHP replacing the same container. Must go!
Along with as many templates that require custom stylings to make them work. The whole purpose for doing this is to slim theming down to mostly all bootstrap classes! This requires many rewrites to BS templates and the way we are going about this is still just massaging these templates needing rewritten.
@tywrenn Great job btw! We're heading in the direction I envisioned with importing a base bootstrap theme and building on that. However, I don't envision our theming engine going away unless i'm not seeing it.
@bradymiller Thanks for taking care of keeping us current with master. A big worry when starting this. Also, I don't see a reason to keep a compact and full tabs theme. One or the other, I pick compact. Besides why would we want all that real estate anyway! It gets in the way of the business end of openemr.

Biggest action item for now to keep from spinning wheels is to re template to BS4 with at minimum using dark and light classes and getting us consistent throughout. Losing custom classes along the way.

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Dec 21, 2019

@sjpadgett I definitely can say there is no way we can completely remove the theming engine based upon what I've seen.

As far as compact and full themes I think there is definitely some people who like the full style and some who like the compact style. Getting rid of it doesn't make sense to me but there are a TON of classes that don't need to be in the compaction style. If we compacted the navbar by only a few class changes, that would be ideal. I personally like the full style layout as it's style looks like Bootstrap's style.

Eventually the only thing separating themes will be a few SASS variable changes here and there which will be quite awesome.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Dec 21, 2019

As far as compact and full themes

If you want to put in the work, okay I guess. Remember though, developers love their screen real estate and the more that is taken by a changing information bar makes it hard to design screens. This is why we end up with scrolls depending on theme. Like it or not, good design practice says that static patient status bar should be consistent in size.

You're right, it will be awesome and welcomed.

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Dec 22, 2019

@sjpadgett @bradymiller Found another way as described here to get both compact and full to work. Turns out we don't have to use SASS to grab the theme colors for tabs 😂. I'll be doing it this way with CSS variables instead to allow us to migrate much easier.

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Dec 22, 2019

Like it or not, good design practice says that static patient status bar should be consistent in size.

That's fine and it will be handy everywhere but, let me try to make this engineering point again by example.
Let's take encounter tabs. That screen was designed for the size of compact(or less) so there aren't any scroll bars however, when full theme is used whala, scrolls.
Though minor it does mean modal touch actions are affected when that scroll is present. I programmed this away best I could but it is hacky imo.

Many other instances of this. The sizing of that top static patient summary bar/header should always be a size one can count on. This is simply good design practice.

This is where all other Admin's need to chime in. You leave all this to me, i'll do it my way.
I'm actually kind of mythed that for such an important change to project it doesn't generate more input...

@stephenwaite

This comment has been minimized.

Copy link
Member

@stephenwaite stephenwaite commented Dec 22, 2019

not an admin but if that top static patient summary bar could somehow be folded into each tab so that each tab was a patient and had sub tabs... openemr 6.0 :)

@sjpadgett

This comment has been minimized.

Copy link
Member

@sjpadgett sjpadgett commented Dec 22, 2019

That'd be great but we'd run up against session management issues. We're having trouble enough now with session persistence with just one patient.
Session management(as such) would need to come out of globals and a true session manager written.

@bradymiller

This comment has been minimized.

Copy link
Member

@bradymiller bradymiller commented Dec 22, 2019

Regarding sessions, that will be another large project, but is something we'll need to attack at some point in the future, since I am guessing it's only a matter of time until browsers do not allow viewing/modification of the session cookies (this is what OpenEMR does to support multiple browser tabs with different sessions):
#2556

@bradymiller

This comment has been minimized.

Copy link
Member

@bradymiller bradymiller commented Dec 22, 2019

Regarding Full versus Compact tab theme, I've noted a generation gap on this regarding preference :)
Age < 40 prefer Full
Age 40-50 ok with either Full or Compact
Age > 50 prefer Compact

@bradymiller

This comment has been minimized.

Copy link
Member

@bradymiller bradymiller commented Dec 22, 2019

btw, I wholeheartedly agree on goal of optimizing real estate no matter what theme is chosen.

tywrenn added a commit to tywrenn/openemr that referenced this issue Dec 22, 2019
tywrenn added a commit to tywrenn/openemr that referenced this issue Dec 24, 2019
tywrenn added a commit to tywrenn/openemr that referenced this issue Jan 4, 2020
@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Jan 5, 2020

Updated to-do list.

tywrenn added a commit to tywrenn/openemr that referenced this issue Jan 5, 2020
- Fixes navbars in the billing area
- Flat styles are now fixed temporarily (will be removed once we start creating new themes)
- Fixes ccr `use` class order
- Cleanup code in the demographics area
- Adds core header to `pnotes_full_add.php`
- Loads theme defaults into style manila and other styles where its missing
- Replaces a few colors (still need community help to determine what colors go best on what
tywrenn added a commit to tywrenn/openemr that referenced this issue Jan 6, 2020
tywrenn added a commit to tywrenn/openemr that referenced this issue Jan 8, 2020
tywrenn added a commit to tywrenn/openemr that referenced this issue Jan 9, 2020
@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Jan 9, 2020

Updated to-do list

@tywrenn

This comment has been minimized.

Copy link
Contributor Author

@tywrenn tywrenn commented Jan 11, 2020

Updated to-do list. I'm hoping to get this issue done by the end of the month.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.