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

Fix #216: Use OptionMenu for large category sets #512

Merged
merged 1 commit into from
Jun 8, 2020
Merged

Fix #216: Use OptionMenu for large category sets #512

merged 1 commit into from
Jun 8, 2020

Conversation

openjck
Copy link
Contributor

@openjck openjck commented May 21, 2020

No description provided.

Copy link
Contributor

@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.

Looking good @openjck – left a few comments to address.

I am noticing that the alignment of these menu items are a bit off. The icon & swatch align, but the number looks like it is not aligned with the rest. Is this something that is addressable here in GLAM? If not, then we should re-create this use-case in graph-paper in storybook to make sure it works as expected.

Screen Shot 2020-05-26 at 9 24 46 AM

There is a merge conflict here that will need to be addressed, but that should be straightforward.

src/components/explore/ProportionExplorerView.svelte Outdated Show resolved Hide resolved
src/components/explore/ProportionExplorerView.svelte Outdated Show resolved Hide resolved
src/components/explore/ProportionExplorerView.svelte Outdated Show resolved Hide resolved
src/components/explore/ProportionExplorerView.svelte Outdated Show resolved Hide resolved
@openjck
Copy link
Contributor Author

openjck commented May 27, 2020

I am noticing that the alignment of these menu items are a bit off.

That's a quick fix in graph-paper. I'll submit a PR to address that now. I don't think we need to block merging this PR on that, but we will get the alignment fixed for free once optionmenu is upgraded.

@openjck
Copy link
Contributor Author

openjck commented May 27, 2020

Here's the graph-paper PR:

graph-paper-org/graph-paper#148

@hamilton
Copy link
Contributor

Thanks for the changes. I made one suggestion about the color choice. 400 feels very dark, and 300 would probably suffice here.

Is there any wy we can have the colored buckets (which are the ones with more counts than the others) at the top of the sortedInactiveBuckets list? As it is now, I clicked on one then had to scroll far to find it again. I think this is a pretty quick change, since you can just keep the inactive buckets with color assignemnts toward the top of that list.

If those makee sense, feel free to make those changes and merge at your discretion. Thanks!

@hamilton hamilton self-requested a review May 28, 2020 01:30
Copy link
Contributor

@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.

suggested changes in previous comment

src/config.json Outdated Show resolved Hide resolved
@openjck openjck merged commit aa00eae into mozilla:master Jun 8, 2020
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