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

Centralizing parent and child theme list and heading styles into SASS partials #165

Merged
merged 9 commits into from
Nov 6, 2024

Conversation

djanelle-mit
Copy link

@djanelle-mit djanelle-mit commented Oct 31, 2024

Previously, our list and heading styles were defined in the wordpress style.css, and each theme had slightly different values. This work removes the references in those style.css files, and moves them to the typography sass partial.

Changes were also made to better typeset the following for readability and legibility:

  • Lists
  • Headings
  • Paragraphs

This work also lays the foundational work to be able to potentially make design system changes in one place in the future.

Only notable difference: our child themes H1s are small all caps overlines while H1s in the parent theme were not. I kept this difference to avoid breaking things downstream. I also had to update some h3 styles on smaller device breakpoints to match how production was set up. I'd like to revisit those when we revisit our typeface.

Developer

Stylesheets

  • Any theme or plugin whose stylesheets have changed has had its version
    string incremented.

Secrets

  • All new secrets have been added to Pantheon tiers
  • Relevant secrets have been updated in Github Actions
  • All new secrets documented in README

Documentation

  • Project documentation has been updated
  • No documentation changes are needed

Accessibility

  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)

Stakeholder approval

  • Stakeholder approval has been confirmed
  • Stakeholder approval is not needed

Dependencies

YES | NO dependencies are updated

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • The changes have been verified
  • The documentation has been updated or is unnecessary
  • New dependencies are appropriate or there were no changes

Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this change philosophically, but want to note one potential impact, and raise one question for discussion as we proceed:

  1. The impact is on the front page, as noted below. If UX doesn't mind the change, then I'm not going to stand against it, but I do want to make sure it's been noted.

  2. The question for discussion is around how we handle version strings within this monorepo. Every plugin and theme that we've written will have a version string, and looking at the child and parent theme now I see that we may not be treating the strings the same. I can see arguments for many conventions, and don't want to come out with a pronouncement without talking with you about it

In general, though, I'm comfortable proceeding with this change as you talked about on Slack - knowing that some things will change, and committing to work through the details as we go.

:shipit: if you're comfortable...

a {
font-weight: 400;
}
margin: 15px 0 10px;
Copy link
Member

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.

Screenshot 2024-11-04 at 10 26 35 AM

Copy link
Author

@djanelle-mit djanelle-mit Nov 4, 2024

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

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.

@@ -1,7 +1,7 @@
/*
Theme Name: MITlib Child
Author: MIT Libraries
Version: 1.0.2
Version: 1.0.3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you're changing the version string for both themes, which is the right thing to do - no changes needed.

That said, I also wanted to mention that we are free to follow any convention we want with these version strings. There's nothing magical about semantic versioning here, and you saw that we don't use the same convention with the parent theme. Ideally we'd use the same convention for all internal version strings in this repo?

If you have thoughts, I'm happy to hear them. We can also talk about what practices we follow for other platforms if that would be helpful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Unless we're going to be doing a lot of updates I'm not sure if we need three layers of version numbers. If we're not worried about leaving room to increment the first number when there's a large update (is that the key component of semantic versioning?) then either path makes sense to me.

It seems like being consistent would be best regardless of which direction we go. What do we do outside of Wordpress?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention we're following with the child theme - but not in a super strict sense, I don't think - is semantic versioning. That makes the most sense to me for software packages which have an external audience - which doesn't apply to anything we're really doing with WordPress.

Outside of WordPress, we've been trending toward simplicity lately. We've done X.Y versioning for things like ETD, which did go through an extensive rewrite between versions 1 and 2 (in hindsight that probably should have just been a new application and codebase, though). Our other big Rails apps, Bento and the TIMDEX API, all have an X.Y format but we've specifically chosen to avoid touching the X digit at several points in the past. Same with TACOS - although we haven't ruled out going from v0.x to v1.x at some point when we've got all the bits we intend to include in place.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense... For the case of X.Y, if you get to X.9 and there's another update... would you just start adding digits? That would be my only concern is that you run out of runway if you're intentionally trying to avoid incrementing X.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that scenario, you go from x.9 to x.10 - the whole sequence is still a string, with only the individual digits treated numerically.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I think that route seems simplest and I'm all for keeping things simple. I can collapse the child theme version to 1.3 with my next round of changes.

This commit splits out overrides for typography from the typography partial into existing partials for locations, home, and hours.

This commit also includes a couple new overrides for home that were needed to bring centralized typography in line with produciton for the homepage.

** Relevant ticket(s):
https://mitlibraries.atlassian.net/browse/PW-112

** How does this address that need:

*

** Document any side effects to this change:
None observed.
*
@djanelle-mit
Copy link
Author

@matt-bernhardt Alrighty, changes are up. Two commits:

  1. First updates the version number from semantic to X.Y
  2. Second splits out the overrides into their respective partials, and I added the fixes to the homepage in the _home partial.

Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me - I see the front page override in that module, and it looks like the result is good. I also see the version scheme change, which brings it inline with the other internal version numbers.

Thanks for working through all this!

:shipit:

@djanelle-mit djanelle-mit merged commit e63e481 into master Nov 6, 2024
4 checks passed
@djanelle-mit djanelle-mit deleted the pw-112 branch November 6, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants