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

Add postcss-loader and autoprefixer dependencies #8821

Merged
merged 1 commit into from Oct 21, 2019

Conversation

WilliamHolden
Copy link
Contributor

@WilliamHolden WilliamHolden commented Oct 17, 2019

I noticed that some of the bootstrap components were not rendering correctly and had some inconsistencies between browsers. I tracked it down to the fact that we are not using an autoprefixer. This pull request adds the post-css and autoprefixer dependencies so that bootstrap works properly on all browsers. This prefixer is recommended by Google and used in Twitter and Alibaba. The autoprefixer is also listed in the minimal setup requirements for bootstrap in the documentation here:

https://getbootstrap.com/docs/4.0/getting-started/webpack/

Ironically I also noticed an error in the bootstrap documentation which states that you should also include require('precss') in the loader. I was skeptical that we needed that module so I researched and found the following PR(removing the precss requirement) into the Bootstrap repo which for some reason they never merged even though it was approved by one of the developers twbs/bootstrap#24671.

@martenson
Copy link
Member

They seem to have actually merged it later, but it did not make it to the release yet?
twbs/bootstrap#27484

@WilliamHolden
Copy link
Contributor Author

WilliamHolden commented Oct 17, 2019

@martenson you're right! It seems to have made it into later versions of the v4 documentation but not older versions. It appears they just have some inconsistencies in their documentation for some reason.

If you type "webpack bootstrap" or "webpack bootstrap 4" into google, the following link is what pops up which doesn't have the updated precss docs.
https://getbootstrap.com/docs/4.0/getting-started/webpack/

However if you type "webpack bootstrap 4.3" then you get the latest version of their docs here which has the correct precss docs.
https://getbootstrap.com/docs/4.3/getting-started/webpack/

Either way, we are using 4.3 so we should be fine

@galaxybot galaxybot added this to the 20.01 milestone Oct 17, 2019
@Nerdinacan
Copy link
Contributor

Nerdinacan commented Oct 18, 2019

Good catch. I know our browser target spectrum is pretty narrow, but it's such an easy addition. Easy to do that is, hard to diagnose bootstrap's many dumpster fires, and we should have it for our own styling anyway.

@WilliamHolden
Copy link
Contributor Author

WilliamHolden commented Oct 18, 2019

Thanks, definitely agree there! Also important to note that this change should be included no matter how small our browser target spectrum is. Not including it is a bug. For example, if you try to use the bootstrap .custom-select class(Used by bootstrap-vue select element), The <select> element will render with two arrows on the latest version of chrome.

Screen Shot 2019-10-18 at 2 40 02 PM

The reason for this is that this class has the attribute appearance: none which has no browser support. This change will add prefixes including-webkit-appearance: none; which is supported and fixes the above issue. I noticed many other issues on chrome too.

@Nerdinacan
Copy link
Contributor

Ahhh hah! Yeah I saw that style bug but chalked it up to one of bootstrap's many deficiencies. Good work in hunting that down.

@dannon dannon merged commit 40b064e into galaxyproject:dev Oct 21, 2019
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

5 participants