forked from jspellman814/wordpress-composer-managed
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Centralizing parent and child theme list and heading styles into SASS partials #165
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
31789f3
Updated style for child theme lists to match parent
djanelle-mit 750e2b7
Centralized list styles in SCSS
djanelle-mit 2a354e4
Tweaks to lists and centralizing heading styles
djanelle-mit 9b2d296
Tweaks to line height and spacing between list items
djanelle-mit b731047
Swapped margin to padding to avoid margin collapse on chrome
djanelle-mit ae6f426
Incrementing versions for parent and child theme style sheets
djanelle-mit 4d99c29
Removing errant end comment
djanelle-mit ee99a44
Changed child theme version from semantic to X.Y
djanelle-mit 1e2d482
** Why are these changes being introduced:
djanelle-mit File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
5 changes: 5 additions & 0 deletions
5
web/app/themes/mitlib-parent/css/scss/partials/_locations.scss
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule appears to impact the front page in a way we may not be comfortable with - there are h3s on the hours block which pick up this margin in a way that seems noteworthy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yes, this must have changed in the latest iteration. I also noticed that the size of the H2s also shifted, so I'll probably need to untangle this a bit more.
From a practical standpoint, is it worth implementing the page-specific styles in the partial files of the appropriate name? For example, using
_home.scss
for the homepage specific heading styles? Keep the typography file completely global? I can move the page-specific overrides on like 46 of_typography.scss
, too. I think that makes sense to me over grouping all the heading-specific overrides into a section of typography.Either way, I think I'll need another round of iteration on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we have a
_home.scss
module already, I can see a good argument for putting any page-specific styles there, and leaving the basic thrust of your cleanup in place as you've built it so far. That would let us put all the rules for a given feature in the same place, rather than checking each base module for any overrides for a given feature.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option, which I mention only for completeness and not because I favor it, would be to put the styles in the
mitlib-pull-hours
plugin which defines this specific widget. I'm not as big a fan of that, however, because that puts on the road to having styles in both the themes and in the various plugins which contribute to the UI - which feels worse from a maintenance perspective.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I'll lean into the modules for specific pages, I think that's the right approach.
I like the idea of trying to keep the distinction that plugins only provide content/functionality and lean on the theme for the styling (as much as we're able to). I suppose if we introduce something really novel and we're confident the styling should be consistent in all the places it appears, we could consider putting the styles with the plugin, but that seems like it could get messy quickly.