-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Copy ID / Delete functionality to My Queries page #202
Add Copy ID / Delete functionality to My Queries page #202
Conversation
query-connector/src/app/queryBuilding/dataState/UserQueriesDisplay.tsx
Outdated
Show resolved
Hide resolved
Hi @robertandremitchell I checked out the new build. Is this what it should look like? |
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Couple design things I spotted, but shouldn't block anything:
On my browser the toast text looks disproportionately large/different from your screenshots in the PR description; maybe just needs a different headingLevel passed in?
I didn't see anything in Figma for the confirmation modal, but the serif font in the modal header doesn't match the other serif text on the page:
query-connector/src/app/query/designSystem/redirectToast/RedirectToast.tsx
Outdated
Show resolved
Hide resolved
Thanks! re: the different toast, this is expected based on @fzhao99 's work here #204 (comment) to leverage/match the existing toast for the demo customize query work? @mikang , are you still okay with the presentation of the toast? for the modal header, I've updated the .scss to the appropriate font for modal header. We may need to revisit if the Modals end up differing on header font/formatting, but as the inaugural Modal...uh, first come, first serve? 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
PULL REQUEST
Summary
Copy ID
Delete
Edit
query_data
from the backend and render it on the edit-value-set page.Related Issue
Fixes #136, partially
Additional Information
I leaned toward moving the Modal and Toast code to a
utils
. I had some issues with the existing code for either the Modal or redirectToast working on this page.The existing Modal does not currently accept more button use cases; we could probably get away with just have
Delete
, but I took a stab at expanding to allow multiple buttons.The Toast I was unable to get to render at all, unfortunately. The code for the toast is fairly lightweight; however, it could be worth debugging just to avoid pain down the line. At the very least, it probably makes sense to move the copy just to make that easier to update, as this text seems liable to change.
Anything else the review team should know?
@mikang , a few questions about the above:
Checklist