Add labels to Curriculum Catalog Card by megcrenshaw · Pull Request #51304 · code-dot-org/code-dot-org

megcrenshaw · 2023-04-12T20:37:48Z

This PR does two things:

Adds new tag styling as indicated in the Redline. Instructions on what to do on overflow are in this thread. I did need to make the cards longer to accommodate the extra space needed for the tags.
Ensures tags are in the ordered specified in the product spec.

From Redline:

On webpage:

Two-line title:

No topics or subjects (no picture loaded yet):

Ellipse:

Non-English:

With hover (but will update to use Tooltip):
https://user-images.githubusercontent.com/9142121/232040244-051289df-e7af-40bd-972a-aa5c5fe59e15.mov

Links

Testing story

Deployment strategy

Follow-up work

Tooltip on hover for both the first tag and the remaining tags: https://codedotorg.atlassian.net/browse/ACQ-524

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-14T13:54:53Z

Need to change the hover styling ... will re-request once that's implemented

megcrenshaw · 2023-04-14T19:23:53Z

Implementing the tooltip is going to require more work to make it accessible. I'm splitting that out into a separate ticket because this PR is big enough. https://codedotorg.atlassian.net/browse/ACQ-524

kelbyhawn · 2023-04-14T20:23:43Z

This looks great! Will the tooltip list out all of the topics stacked vertically?

davidsbailey · 2023-04-14T20:57:08Z

This PR does two things:

Adds new tag styling as indicated in the Redline. Instructions on what to do on overflow are in this thread. I did need to make the cards longer to accommodate the extra space needed for the tags.

Ensures tags are in the ordered specified in the product spec.

sorry but what are tags in this context?

megcrenshaw · 2023-04-17T12:23:02Z

This looks great! Will the tooltip list out all of the topics stacked vertically?

Yes! The first pass of that looks like (just updated the follow-up work section to make this clear)

megcrenshaw · 2023-04-17T12:28:08Z

This PR does two things:

Adds new tag styling as indicated in the Redline. Instructions on what to do on overflow are in this thread. I did need to make the cards longer to accommodate the extra space needed for the tags.

Ensures tags are in the ordered specified in the product spec.

sorry but what are tags in this context?

Oh yikes! I've just been calling these tags in my head for forever for no real reason. Tags are the blue "bubbles" underneath the image that show a category name –– they could be a subject or a topic from the backend. I'm only using the term tag to describe the styles, but would category be a more clear name here? I hesitate to do that because category is a separate field on the backend.

Another potential follow-up is that on the backend, these fields are called cs_topic and school_subject which is misleading because there could be multiple topics and subjects, formatted as a CSV. Adding some validation here would be nice. Do you think it also warrants a name change to the fields to be clear that there could be multiple?

davidsbailey · 2023-04-17T21:35:31Z

sorry but what are tags in this context?

Oh yikes! I've just been calling these tags in my head for forever for no real reason. Tags are the blue "bubbles" underneath the image that show a category name –– they could be a subject or a topic from the backend. I'm only using the term tag to describe the styles, but would category be a more clear name here? I hesitate to do that because category is a separate field on the backend.

No worries! in this comment, I was mostly asking to help figure out what I was reviewing before digging in.

having read through a bit, I would suggest either "label", which sounds self-documenting to me, or keeping "tag" and adding a well-placed comment indicating what it means.

Another potential follow-up is that on the backend, these fields are called cs_topic and school_subject which is misleading because there could be multiple topics and subjects, formatted as a CSV. Adding some validation here would be nice. Do you think it also warrants a name change to the fields to be clear that there could be multiple?

if I'm understanding correctly, the change would be to rename these fields to cs_topics and school_subjects on the backend and in the data that gets sent to the client? yes, I think that would be a good idea, while this is still unlaunched and therefore easier to change.

davidsbailey

LGTM after adding a brief comment on what a tag is, or please request re-review if doing a bigger change like switching to "label".

megcrenshaw · 2023-04-18T13:58:38Z

LGTM after adding a brief comment on what a tag is, or please request re-review if doing a bigger change like switching to "label".

I like the labels change!

change from tag to labels: 5439436
Other commits can be seen in the history, but I did rebase due to merge conflicts. I also changed styling to use child selectors rather than classnames (47ed60d), and there was a bug when no labels were present, so I fixed that (cf67a04) and added another story (cb498cd)

kelbyhawn · 2023-04-18T18:48:18Z

Not blocking, but I just noticed the non-English stacked buttons height (on line 127) .buttonsContainer_notEnglish doesn't match the English button height — both should be height: 40px.

This was actually intentional ... if it's larger, the card needs to be even longer to have some space between the grades and the button

Here's what it looks like now

I'll ask in the design channel about this!

Actually I just made the executive decision to change it with a larger height

Nice! I think this was the right call 👍

kelbyhawn

Looks great, and I like the tags >> labels change!

davidsbailey

looks great, thanks for updating! just two tiny nits below

davidsbailey · 2023-04-18T19:05:09Z

tag -> label

davidsbailey · 2023-04-18T19:05:16Z

tag -> label

This reverts commit 906960f747ccb35170916958d29bafb704f733fc.

megcrenshaw force-pushed the meg/add-tag-styling-curriculum-card branch from c3c7ca7 to d3bb661 Compare April 14, 2023 02:20

megcrenshaw marked this pull request as ready for review April 14, 2023 12:21

megcrenshaw requested review from a team and kelbyhawn and removed request for a team and kelbyhawn April 14, 2023 12:21

megcrenshaw marked this pull request as draft April 14, 2023 13:54

megcrenshaw marked this pull request as ready for review April 14, 2023 19:24

megcrenshaw requested review from a team and kelbyhawn April 14, 2023 19:24

davidsbailey approved these changes Apr 17, 2023

View reviewed changes

megcrenshaw changed the title ~~Add tags to Curriculum Catalog Card~~ Add labels to Curriculum Catalog Card Apr 17, 2023

megcrenshaw force-pushed the meg/add-tag-styling-curriculum-card branch from b507c11 to 47ed60d Compare April 18, 2023 13:49

megcrenshaw requested a review from davidsbailey April 18, 2023 13:58

kelbyhawn reviewed Apr 18, 2023

View reviewed changes

kelbyhawn approved these changes Apr 18, 2023

View reviewed changes

davidsbailey approved these changes Apr 18, 2023

View reviewed changes

Meg Crenshaw added 5 commits April 18, 2023 16:51

Add tags

7be4c0b

Change storybook to match redline

e63a2f5

Add tags styling

e294965

Show full word on hover

8ce814c

Add subjects and topics order

7382a0e

Meg Crenshaw added 12 commits April 18, 2023 16:54

Add prioritization order to topics and subjects

b13f4b7

Adjust styles and only show tags styling if tags present

72160d9

Fix classname

6985aff

Conditionally render section instead of classname

5edf75b

Add no tags card to storybook

c54c469

Change tag to label

18635fc

Use child selector instead of separate classname

17ffa0c

Fix placement of translatability tag

966434c

Revert "Fix placement of translatability tag"

b808ecf

This reverts commit 906960f747ccb35170916958d29bafb704f733fc.

Conditionally render labels but keep container present

8b4e099

Add story for not translated

345432f

Make button height standard for non-english view

6e6f786

megcrenshaw force-pushed the meg/add-tag-styling-curriculum-card branch from cb498cd to 6e6f786 Compare April 18, 2023 21:08

megcrenshaw merged commit 0cc37cc into staging Apr 19, 2023

megcrenshaw deleted the meg/add-tag-styling-curriculum-card branch April 19, 2023 11:54

megcrenshaw mentioned this pull request May 31, 2023

Add tooltip on Curriculum Catalog Card Labels #52163

Merged

8 tasks

Uh oh!

Conversation

megcrenshaw commented Apr 12, 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!

megcrenshaw commented Apr 14, 2023

Uh oh!

megcrenshaw commented Apr 14, 2023

Uh oh!

kelbyhawn commented Apr 14, 2023

Uh oh!

davidsbailey commented Apr 14, 2023

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

davidsbailey commented Apr 17, 2023

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

kelbyhawn Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

megcrenshaw Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

megcrenshaw Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

megcrenshaw Apr 18, 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!

kelbyhawn Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

kelbyhawn left a comment

Choose a reason for hiding this comment

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

davidsbailey Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

davidsbailey Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

megcrenshaw commented Apr 12, 2023 •

edited

Loading

megcrenshaw commented Apr 17, 2023 •

edited

Loading

megcrenshaw commented Apr 17, 2023 •

edited

Loading

megcrenshaw commented Apr 18, 2023 •

edited

Loading

megcrenshaw Apr 18, 2023 •

edited

Loading