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

Implement highlightable description #380

Merged

Conversation

ducku
Copy link
Collaborator

@ducku ducku commented Dec 10, 2023

Puts Description, if available, as text below Go button.

Closes #378

Copy link
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

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

It looks like this should work. I think the extra argument to handleRegionChange is the wrong way to do it, but I think to do it the right way it has to be coordinated with @shreyasun and what she ends up doing for the left/right button work.

if (regionObject) {
regionValue = regionObject.value
}
handleRegionChange(regionValue, regionToDesc.get(regionValue));
Copy link
Member

Choose a reason for hiding this comment

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

I feel like the regionToDesc lookup might really want to be in handleRegionChange? So it doesn't need a new argument? But we kind of maintain the whole database of regions in RegionInput and it would be odd to do it twice.

I think Shreya is working on the left/right button jump-to-next-region thing in #354 though and that requires having more of an understanding of the region database in the header form, so I think this sort of thing is moving out of RegionInput in the medium term.

@adamnovak
Copy link
Member

Here's a screenshot of the text on the page, under the go button, with its heading: Screenshot 2023-12-12 at 12 27 37

It's tied to what region you have selected/typed, not to what region is currently displayed, which might be a little confusing? But it's probably fine.

@adamnovak adamnovak merged commit 2ac1fb3 into vgteam:master Dec 12, 2023
1 check passed
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.

Move region description tooltips to highlightable text on the page
2 participants