Add non-English styling to Curriculum Catalog Card by megcrenshaw · Pull Request #51202 · code-dot-org/code-dot-org

megcrenshaw · 2023-04-07T13:44:02Z

Adds non-english styling of the card, which (a) changes the button layout, and (b) makes the card longer to accommodate for the button layout. Molly has approved the design. Also added the correct focus state (see thread)

English card:

Non-English card:

Focus state for button:

This work also flattens styles. See initial discussion of flattening styles here #51099 (comment), and compare the non-flattened styles to the flattened styles (split view comparison here)

Links

Testing story

No unit tests added here, as it's only styling modifications. Adding an eyes test to capture the Curriculum Catalog Page (which includes the card) will be part of the work before launching.

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

megcrenshaw · 2023-04-07T14:51:19Z

+  margin: 0 0 0.5em 0;
+  align-items: center;
+
+  .iconWithDescriptionText {


I intentionally left this nested to visually connect it with the iconWithDescription. In the BEM world, I would've called this icon-with-description__text because it's an element. But with the need for camel casing in JSX for the style.keyName syntax, I stuck with camel case here too.

I realize this is not consistent with the rest of the flattening –– open to more discussion about this!

Sidenote: Looks like there are some libraries out for using CSS modules with BEM, but we haven't normed on using BEM conventions, and combining BEM with flattening the styles could also get messy.

I intentionally left this nested to visually connect it with the iconWithDescription. In the BEM world, I would've called this icon-with-description__text because it's an element. But with the need for camel casing in JSX for the style.keyName syntax, I stuck with camel case here too.

BEM looks quite interesting! If the only thing stopping you is the camel casing, you should be able to do something like style['icon-with-description__text']. I suppose it is a tradeoff. I am interested in your thoughts on whether we should push further in the BEM direction.

I realize this is not consistent with the rest of the flattening –– open to more discussion about this!

This seems fine... after discussion I'm fine with being flexible on whether styles are flattened.

Sidenote: Looks like there are some libraries out for using CSS modules with BEM, but we haven't normed on using BEM conventions, and combining BEM with flattening the styles could also get messy.

If we wanted to adopt BEM, I wouldn't let my initial opinion on nested styles get in the way. adopting BEM seems like a much bigger benefit than any of my arguments against.

I didn't want to open the BEM can of worms tehe ... but some initial thoughts

Our Styleguide says to use kebab-case for styling. But we aren't really following this, e.g. the style['icon-with-description__text'] syntax is only present in 6 files in our repo.

I asked about the camelCase vs kebab-case in the summer swarm (thread), and turns out that CSS Modules recommends CamelCase. Perhaps the easiest lift right now is to update the styleguide.

We could enable a stylelint rule for camelCase, but if my initial stylelint attempt for linting the ID is any indication, it would be a pretty big PR and would require some manual editing, as classnames show up in JS files too.

There are lots of BEM naming conventions, but they all involve the -, which would require the style['...'] syntax. I don't think this is blocking, but it would negate the use of the stylelint rule.

I do like the BEM standards, but I used it in my last company and it required a fair bit of upskilling across the team to make it work (we were also basically starting a new repo, so figuring out what to do with existing styles wasn't an issue). I'm not sure it's worth the hassle? Dave, what are the pros you're excited about?

CC @sanchitmalhotra126 and @mikeharv if you have thoughts on this ...

one more thought on adopting BEM is that if we want to head in that direction, it would be super handy to have a linter to help us. it's possible these pieces could work together to accomplish this:

https://github.com/postcss/postcss-bem-linter

https://stylelint.io/

https://www.npmjs.com/package/stylelint-selector-bem-pattern

I would love to support you in pursuing this if you are interested!

We put Stylelint in our repo this summer! See initial PR #47591 and current config.

Nice finds with the BEM-specific linters. That negates a lot of my points above. I suppose the questions remaining would be

Do we want to adopt the BEM conventions?

If so, what do we do with existing styles?

I'll defer to others on whether we want to head in the BEM direction. my input is that it'd be nice to have better style consistency in CSS, especially if it can be enforced via linting.

if using a linter, these would just be linter exceptions (via code comments) for existing styles which are hard to update. having said that... updating classnames can be tricky because you have to be very sure you are finding every instance of each classname you update or pages could break (styling and functionality), so there could be quite a lot of exceptions at first.

one idea would be to pick a direction, and then add linting only for css modules, which we don't have very many of yet. then, as our use of css modules grows, our css will grow in the direction of a more consistent style.

I like the idea of only linting modules for now and using BEM in those. I'll ask around a bit more, as I have lots of linter wishes!

davidsbailey

looks great!

one more thought on adopting BEM is that if we want to head in that direction, it would be super handy to have a linter to help us. it's possible these pieces could work together to accomplish this:

I would love to support you in pursuing this if you are interested!

davidsbailey · 2023-04-07T16:21:22Z

  topics,
-  isTranslated
+  isTranslated,
+  language


are you aware of any place in the catalog we will need the language for anything besides determining whether the page is in english? if not, my recommendation would be "YAGNI" and just assume isEnglish will be passed into this component.

Good point –– fixed!

davidsbailey · 2023-04-07T16:36:22Z

+  margin: 0 0 0.5em 0;
+  align-items: center;
+
+  .iconWithDescriptionText {


I intentionally left this nested to visually connect it with the iconWithDescription. In the BEM world, I would've called this icon-with-description__text because it's an element. But with the need for camel casing in JSX for the style.keyName syntax, I stuck with camel case here too.

BEM looks quite interesting! If the only thing stopping you is the camel casing, you should be able to do something like style['icon-with-description__text']. I suppose it is a tradeoff. I am interested in your thoughts on whether we should push further in the BEM direction.

I realize this is not consistent with the rest of the flattening –– open to more discussion about this!

This seems fine... after discussion I'm fine with being flexible on whether styles are flattened.

Sidenote: Looks like there are some libraries out for using CSS modules with BEM, but we haven't normed on using BEM conventions, and combining BEM with flattening the styles could also get messy.

If we wanted to adopt BEM, I wouldn't let my initial opinion on nested styles get in the way. adopting BEM seems like a much bigger benefit than any of my arguments against.

kelbyhawn · 2023-04-10T15:14:03Z

    &:focus {
-      border-color: $brand_primary_default;
+      outline: $brand_primary_default solid 2px;
+      outline-offset: 2px;


Nit and def not blocking: you could make a mixin for the focus state since it's repeatable 🤓

Oh brilliant! And good mixin practice for me too.

Okay I may or may not have copied exactly what you did in the pegasus styles ... but it's done! Good call –– thanks.

Excellent! 🙌

kelbyhawn

Looks great, thanks again for all of the styling updates!

Meg Crenshaw added 3 commits April 7, 2023 09:38

Use 100% width buttons for non-English

1368a8e

Fix test by adding prop

ead074e

Add focus state to buttons

7e4dd02

megcrenshaw changed the title ~~Meg/add non english styling~~ Add non-English styling to Curriculum Catalog Card Apr 7, 2023

Flatten styles and make height longer

1a59eab

megcrenshaw force-pushed the meg/add-non-english-styling branch from 81310b9 to 1a59eab Compare April 7, 2023 13:55

Reverse order of column buttons

826a499

megcrenshaw requested review from a team and kelbyhawn April 7, 2023 14:30

megcrenshaw marked this pull request as ready for review April 7, 2023 14:34

megcrenshaw commented Apr 7, 2023

View reviewed changes

davidsbailey approved these changes Apr 7, 2023

View reviewed changes

Meg Crenshaw added 4 commits April 7, 2023 13:05

Remove blue border-color for button in focus

d48ce69

Change language prop to isEnglish

d5d377f

Put focus states in button module

deb5ead

Fix test and rename file

c39f725

kelbyhawn reviewed Apr 10, 2023

View reviewed changes

kelbyhawn approved these changes Apr 10, 2023

View reviewed changes

Use mixin for focus state

d48afc6

megcrenshaw merged commit feed579 into staging Apr 10, 2023

megcrenshaw deleted the meg/add-non-english-styling branch April 10, 2023 20:59

megcrenshaw mentioned this pull request Jun 8, 2023

Use scss linting #47591

Merged

8 tasks

Uh oh!

Conversation

megcrenshaw commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megcrenshaw Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelbyhawn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

megcrenshaw commented Apr 7, 2023 •

edited

Loading

megcrenshaw Apr 10, 2023 •

edited

Loading