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

In-plot bookmarking #416

Merged
merged 8 commits into from
Nov 7, 2024
Merged

In-plot bookmarking #416

merged 8 commits into from
Nov 7, 2024

Conversation

JakeWags
Copy link
Member

@JakeWags JakeWags commented Oct 31, 2024

Does this PR close any open issues?

Closes #360

Give a longer description of what this PR addresses and why it's needed

This PR adds the ability for the user to click the bookmark star in the plot to bookmark the subset or aggregate row.

Provide pictures/videos of the behavior before and after these changes (optional)

Screen.Recording.2024-10-31.at.10.55.08.AM.mov

Have you added or updated relevant tests?

  • Yes
  • No changes are needed

Have you added or updated relevant documentation?

  • Yes
  • No changes are needed

Are there any additional TODOs before this PR is ready to go?

None

@JakeWags JakeWags requested a review from NateLanza October 31, 2024 16:57
Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for upset2 ready!

Name Link
🔨 Latest commit 98768d6
🔍 Latest deploy log https://app.netlify.com/sites/upset2/deploys/672cf7bce5e8860008c8c22c
😎 Deploy Preview https://deploy-preview-416--upset2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@NateLanza NateLanza left a comment

Choose a reason for hiding this comment

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

Approved with a couple suggested changes.

packages/upset/src/components/Columns/BookmarkStar.tsx Outdated Show resolved Hide resolved
packages/upset/src/components/Columns/BookmarkStar.tsx Outdated Show resolved Hide resolved
@NateLanza NateLanza self-requested a review November 4, 2024 00:22
Copy link
Contributor

@NateLanza NateLanza left a comment

Choose a reason for hiding this comment

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

Actually, sorry, approved the code before looking at the functionality. I don't like that the click only works exactly on the star; I think you should be able to click anywhere in the column. Just requires wrapping the star in a clickable rect.

@JakeWags
Copy link
Member Author

JakeWags commented Nov 4, 2024

Actually, sorry, approved the code before looking at the functionality. I don't like that the click only works exactly on the star; I think you should be able to click anywhere in the column. Just requires wrapping the star in a clickable rect.

@NateLanza Forgot to comment it here, but Jack and I talked about this and I was planning to fix it. Also found a bug where bookmarking through the plot uses the internal id rather than the proper display name. I'll let you know when those are ready

@JakeWags JakeWags requested a review from NateLanza November 4, 2024 18:14
Copy link
Contributor

@NateLanza NateLanza left a comment

Choose a reason for hiding this comment

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

Fix the one selector that isn't camelCase before you merge.

* @param row - The row for which the color is being selected.
* @returns The color associated with the given row.
*/
export const BookmarkedColorSelector = selectorFamily<string, Row>({
Copy link
Contributor

Choose a reason for hiding this comment

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

First char of name should be lowercase?

@JakeWags JakeWags merged commit 02914f9 into main Nov 7, 2024
7 checks passed
@JakeWags JakeWags deleted the 360-in-plot-bookmarking branch November 7, 2024 17:31
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.

Add bookmarking in the plot
2 participants