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

convert select component, consolidate bookmark dialog #5545

Merged
merged 12 commits into from
Sep 13, 2024

Conversation

briangregoryholmes
Copy link
Contributor

No description provided.

@briangregoryholmes briangregoryholmes self-assigned this Aug 28, 2024
@ericpgreen2
Copy link
Contributor

@briangregoryholmes is this ready for review? Mind just fixing the code quality test?

@nishantmonu51 nishantmonu51 added the blocker A release blocker issue that should be resolved before a new release label Sep 3, 2024
@briangregoryholmes briangregoryholmes linked an issue Sep 3, 2024 that may be closed by this pull request
Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

Looks way better aesthetically.

UXQA – I tried the Create Bookmark form, and making a selection in the new Select component caused the dialog to close. See JAM.

Approving so you can merge after fixing.

@briangregoryholmes
Copy link
Contributor Author

briangregoryholmes commented Sep 7, 2024

For the sake of practicality, I've solved this by consolidating the CreateBookmarkDialog and EditBookmarkDialog components into a single component that uses the ShadCN dialog. The previous component, from headless, was closing because it was naively detecting an outside click when selecting a menu item.

There is a lot that has to be done to make our dialog implementations more consistent throughout the application. The changes in this PR are small just to get it over the line.

I also changed the Bookmark input label from Name to Label because it seems like the 1Password plugin wants to autofill fields that are labeled Name.

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

I like the cleanup. Two UXQA findings:

  1. The Bookmark dialog's absolute time range toggle doesn't seem to be respected. It always flips to "on". See Jam
  2. The TimePicker component isn't seeming to create valid times. When I create a scheduled report, the chosen time isn't respected, and the "edit" form is not able to populate the time field correctly. See Jam.

@briangregoryholmes briangregoryholmes changed the title convert select component convert select component, consolidate bookmark dialog Sep 13, 2024
@ericpgreen2 ericpgreen2 removed the blocker A release blocker issue that should be resolved before a new release label Sep 13, 2024
@ericpgreen2
Copy link
Contributor

ericpgreen2 commented Sep 13, 2024

It still seems like the Bookmark toggles aren't being respected. Toggling-on either "Save filters only" or "Absolute time range" is not retained. See Jam.

@briangregoryholmes
Copy link
Contributor Author

Could not reproduce any issue with the filters only toggle, but the other one should be resolved.

@briangregoryholmes briangregoryholmes merged commit 4e18d0b into main Sep 13, 2024
7 checks passed
@briangregoryholmes briangregoryholmes deleted the bgh/select branch September 13, 2024 19:17
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.

Adopt ShadCN's Select component
3 participants