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

Extract custom dropdown to reusable component, fix styles, match zeplin specs, use it in Sim tab #35

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

pjanik
Copy link
Member

@pjanik pjanik commented Aug 26, 2024

https://www.pivotaltracker.com/story/show/188162567

@kswenson, tagging you here, as Joe is not available anymore and Doug is out for some time.

This PR splits the location-picker into the location-picker and a reusable Dropdown component. Then, I used this Dropdown component in the simulation tab. There are also multiple fixes to the dropdown style that affect the original location picker (sizing, fonts, padding, hover and active states, etc.). Hopefully, it's now fully matching the Zeplin specs. I also fixed some bugs, such as the issue where the user sometimes had to select an option twice. I think the only thing missing is the arrow on the right side, which I'll add tomorrow while converting 'My Locations' to a new component.

It was more work than I hoped for, and I'm not sure if a custom dropdown implementation with text search is a good idea long-term (but it was already there), but given that the styling is so specific, maybe it also wouldn't be easy to work with an external library.

I also moved the SCSS files next to the components to match our typical pattern.

…h zeplin specs, use it in Sim tab [PT-188162567]
@pjanik pjanik requested a review from kswenson August 26, 2024 20:00
Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 LGTM -- just one suggestion that need not hold up the PR.

Comment on lines +46 to +49
document.addEventListener("click", handleClickOutside);
return () => {
document.removeEventListener("click", handleClickOutside);
};
Copy link
Member

Choose a reason for hiding this comment

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

Click events are sent fairly late in the life cycle of a mouse event, which leaves lots of opportunity for other code to interfere, e.g. via preventDefault(). Most attempts to handle this case try to intercept the event as early as possible, e.g. by listening to pointer events (which precede touch or mouse events) and passing "true" for capture so that the intercept occurs early. That said, I see that this code just moved here from elsewhere and so predates this PR, so I leave it to your discretion whether to change anything at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally, I agree. You're correct - this isn't my original code. It was already in the repo. I just refactored it into a more reusable component. I did have some doubts about doing everything manually here instead of using an external Dropdown-with-text-search library that might handle these nuances better. But things seem to work so far, and with the project's scope being pretty limited, I think there's a low chance of running into the issues you mentioned.

I actually tested this feature, and it seems fine, so I’ll leave it as is for now to avoid breaking what’s currently working.

@pjanik pjanik merged commit e507951 into main Aug 27, 2024
5 checks passed
@pjanik pjanik deleted the 188162567-custom-dropdown branch August 27, 2024 14:22
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