-
Notifications
You must be signed in to change notification settings - Fork 1
Adding new Alert styles to parent theme #161
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
Conversation
Our current Alerts in Wordpress don't follow our style guide, and aren't consistent inside of Wordpress. The particular example in PW-108 is handcoded and uses inline style. We also now have a new Alert pattern styling as part of our new design system efforts. * ** Relevant ticket(s): PW-108 - https://mitlibraries.atlassian.net/browse/PW-108 * https://mitlibraries.atlassian.net/browse/ ** How does this address that need: This work implements the new alert styling inside of our Wordpress parent theme. The new class is prefixed with "mitlib-" to avoid conflict with styles from other plugins or themes. This particular instance uses the default informational style, but classes have been added to upgrade this to danger, warning, or success in future use cases. * ** Document any side effects to this change: This work uses icons that already existed in FontAwesome. We'll need to upgrade to a new version to use the ideal icons. The icon choices are semantically close and will be fine until we're able to upgrade. *
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 all seems fine to me - I've been able to use the new style locally, and looked over your example page. I'm probably happy to have this merge as-is, but wanted to ask a question or two beforehand.
-
We've talked a little about colors already, but now that there's an implementation I wanted to ask about the relationship between these hexcodes and the ones defined in the colors module (scss/modules/_colors.scss in this theme) . Are these alert colors meant to be bespoke, never repeated elsewhere? Or would it be possible to work with the colors module - either by adding the colors there are specifically meant for alerts (using that as part of the color name, for example), or updating the existing color names to use these hexcodes? I'm not sure that I want to ask for us to wholesale consider the color swatches we use right now, but it also makes me nervous to keep track of colors that have been defined in individual modules here and there.
-
Am I right in thinking that these styles assume the content editor is going to be writing the markup themselves, instead of relying on a WYSIWYG editor or other approach? Has there been a discussion about how to expand the use of these styles into other areas, like the site-wide alerts that are rendered via
js/alerts.js
or the various template-rendered location alerts? My assumption at the moment is that the styles added here are going to end up getting used in those other templates, but I wanted to confirm this.
Ahh that's a really good point... I was thinking that color might be worth addressing as one of the very first steps in centralizing some of these styles, but exactly how we go about doing that is still up in the air. Maybe it would make sense to, as part of this work, codify those variables even if it's just in the Wordpress scope? Or would it make more sense to have a bigger discussion about delivering some variables more globally across Rails/WP and as many of our vended tools as we can? What do you think?
For now, the editor would need to write or copy/paste the markup. I put in a ticket to see if we can get Alerts in the editor in some way (perhaps as a toolbar button), but that would be out of scope for this. I could also see a ticket (or multiple tickets) to apply these styles to all Alert use cases we have. I'd love to do that work soon, too, but it may need to wait until a few other tickets are complete. Edited to add: There's a separate Banner pattern for the larger site-wide alerts that also may need some tickets to implement. |
I've been noodling on this and I think the best course, regardless of what we do next for colors, will be to codify the alerts colors in variables in Wordpress. Then, at least, if it takes a very long time to revisit the colors in a more global sense, we have semantic variables for these. I'll get an update to this with colors added. |
Added an update that uses color variables completely inside _alerts and added new semantic color variables to _colors. I was able to reuse the existing red and yellow variables, but added two new variables specifically for the blue and green in alerts. Long term I'd like those to be refactored into whatever our ideal color variable solution is, but for now that seemed like a non-breaking way to add those specific swatches. I was a little wary adding the button styling in the alerts, but until I'm able to address buttons site-wide, hardcoding the style in the alert class seems like the best approach. |
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.
Thanks! The colors and alerts modules look good now. I agree with all your points about worrying about global color handling down the road, and also about button styling being both needed and a later problem.
Everything you say on my second question makes sense too - glad to know it's on the radar, even if not now. There should be a way to integrate this with the editor. Hopefully.
My only request now is that you seem to have committed the commit template as a file named :
in the repository root - could you remove that, and then this is good to merge?
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.
Thanks for working through this process with me. This looks great!
Of course, thanks for all the help along the way!! |
Our current Alerts in Wordpress don't follow our style guide, and aren't consistent inside of Wordpress. The particular example in PW-108 is handcoded and uses inline style.
We also now have a new Alert pattern styling as part of our new design system efforts. Draft documentation can be found here: https://mitlibraries.atlassian.net/wiki/spaces/DS/pages/4106747930/Alert
Relevant ticket(s):
PW-108 - https://mitlibraries.atlassian.net/browse/PW-108
How does this address that need:
This work implements the new alert styling inside of our Wordpress parent theme. The new class is prefixed with
mitlib-
to avoid conflict with styles from other plugins or themes.The particular instance in the ticket uses the default informational style, but classes have been added to upgrade this to danger, warning, or success in future use cases.
Document any side effects to this change:
This work uses icons that already existed in FontAwesome. We'll need to upgrade to a new version to use the ideal icons. The icon choices are semantically close and stakeholders have approved that this change will be fine to publish as-is, and we can update the icons here when the upgrade is complete.
Developer
Stylesheets
string incremented.
Secrets
Documentation
Accessibility
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
Dependencies
YES | NO dependencies are updated
Code Reviewer
(not just this pull request message)