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

Improve display of labels (fixes #602) #626

Merged
merged 3 commits into from May 28, 2021
Merged

Improve display of labels (fixes #602) #626

merged 3 commits into from May 28, 2021

Conversation

Iinh
Copy link
Contributor

@Iinh Iinh commented May 28, 2021

Storybook for Label.svelte:
CleanShot 2021-05-28 at 08 28 44@2x

Some notes:
I wanted to find a color palette that is color-blind friendly. This article recommends a few different palettes, and I decided to go with the light colors as I find them fit the best with Protocol. The pill design is largely inspired by Github labels.

To make this component more reusable, such that the same text will get the same color, I generate the label color using an A1Z26 cipher. The text is first turned into a number from 1-9, then from this number we can get a color from the color palette map.

However, if for some reason we want to use a specific color for the pill, we can override the above method by setting a labelNumber number. I did this with the deprecated and expired labels.

I'm not sure if this approach is more complicated than necessary, and I'm open to feedback if there's any way we can improve it :)

Pull Request checklist

  • The pull request has a descriptive title (and a reference to an issue it
    fixes, if applicable)
  • All tests and linter checks are passing
  • The pull request is free of merge conflicts

@Iinh Iinh requested a review from wlach May 28, 2021 15:49
@wlach
Copy link
Contributor

wlach commented May 28, 2021

@hamilton you have some expertise in this area, could you comment on the a11y side of things?

Copy link

@hamilton hamilton left a comment

Choose a reason for hiding this comment

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

@wlach asked me to take a look. From an accessibility standpoint, it looks like label-5 has contrast issues with white text. It may be somewhat easier to not allow people to mix and match text + label with this in mind. I don't see any contrast issues with the other ones.

Screen Shot 2021-05-28 at 9 10 55 AM

I left some take-or-leave suggestions as well.

src/components/Label.svelte Outdated Show resolved Hide resolved
src/components/Label.svelte Outdated Show resolved Hide resolved
Copy link
Contributor

@wlach wlach 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 great, I defer to @hamilton's suggestions on the tweaks.

I think we may want to define more "types" of labels in the future (ala what you did with expired and deprecated), but I think this implementation is more than good enough to start with.

src/__snapshots__/storyshots.test.js.snap Show resolved Hide resolved
@Iinh
Copy link
Contributor Author

Iinh commented May 28, 2021

Thanks @hamilton and @wlach for reviewing this PR. I fixed the contrast and padding issue, here are the updated labels:

CleanShot 2021-05-28 at 10 39 03@2x

@hamilton If you don't mind could you share the plugin that you used to catch that contrast issue? I couldn't find the same one in the Firefox add-on store. Thanks :)

@hamilton
Copy link

@linh my pleasure! It's actually built-in to Firefox devtools – look for "Accessibility" in the tabs.

@Iinh Iinh merged commit d33ed7a into main May 28, 2021
@Iinh Iinh deleted the labels branch May 28, 2021 19:48
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.

None yet

3 participants