Conversation
alanmoo
left a comment
alanmoo
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This has a few too many line changes in mofo-bootstrap.scss for me to be comfortable- the comments that were there are useful when reading through the code in the future.
That being said, while this could potentially be useful for us, we've been considering deprecating this project as we're no longer building multiple sites the way we used to. We also don't have solid front-end testing in place to confirm that this won't break anything, so I'm inclined to leave things as they are. You're welcome to update mofo-bootstrap.scss to make this a bit easier to review, but I can't guarantee that we'll have the time to do it.
If you're looking for other ways to contribute to a Mozilla project, can I suggest something with the label Good first issue?
|
Hi @alanmoo |
alanmoo
left a comment
alanmoo
left a comment
There was a problem hiding this comment.
As it is, this looks like it makes breaking changes to foundation.mozilla.org, which relies on this project. So if after the comments below are addressed it still does, this would need to be a new major version release.
src/scss/mofo-bootstrap.scss
Outdated
| // Reset and dependencies | ||
| @import '../../node_modules/bootstrap/scss/normalize'; | ||
| @import '../../node_modules/bootstrap/scss/root'; | ||
| @import '../../node_modules/bootstrap/scss/reboot'; |
There was a problem hiding this comment.
Moving this up has the potential to significantly change the way things work, is there a reason it moved?
There was a problem hiding this comment.
Is according to actual bootstrap.scss from v4.1.3
| @import '../../node_modules/bootstrap/scss/variables'; | ||
| @import './overrides/variables'; | ||
| @import '../../node_modules/bootstrap/scss/mixins'; | ||
| @import './overrides/mixins'; |
There was a problem hiding this comment.
Restored and updated to current bootstrap
|
|
||
| // Reset and dependencies | ||
| @import '../../node_modules/bootstrap/scss/normalize'; | ||
| @import '../../node_modules/bootstrap/scss/root'; |
There was a problem hiding this comment.
Where'd this come from, what does it do? Did bootstrap add it recently or was it something we intentionally left out previously?
There was a problem hiding this comment.
Normalize was dropped into reboot, here twbs/bootstrap@4fa7749#diff-716327f6d9890462da5d6f981f62fcc6
There was a problem hiding this comment.
Seems like reboot/root are two different things?
| @import '../../node_modules/bootstrap/scss/media'; | ||
| @import '../../node_modules/bootstrap/scss/list-group'; | ||
| @import './overrides/list-group'; | ||
| @import '../../node_modules/bootstrap/scss/responsive-embed'; |
There was a problem hiding this comment.
Did Bootstrap remove this?
There was a problem hiding this comment.
This is included in utilities file
AngelFQC
left a comment
AngelFQC
left a comment
There was a problem hiding this comment.
Pull Request updated
You can view the current styles on https://mozillaperu.github.io/mofo-bootstrap/demo/
2 participants
No description provided.