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.

Sign up for GitHub

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

Jump to bottom

Add support for tracking multiple sets of turnip prices #125

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add support for tracking multiple sets of turnip prices #125

wants to merge 6 commits into from

Conversation

JonathanGin52
Copy link

@JonathanGin52 JonathanGin52 commented May 24, 2020 •
edited
Loading

What is this PR trying to do?

This PR adds support for tracking multiple sets of turnip prices in parallel. For simplicity, each set of turnip prices is referred to as an "island". The changes include:

  • Introducing tabs to navigate between islands
  • Use a unique localStorage filter key for each island
  • Store tab list in localStorage
  • Support for dynamically adding / removing tabs

Demo

tabs

Risks

  • I'm no desiginer by any stretch, so this isn't the prettiest implementation 😬
  • Delete tab icon a bit finicky, sometimes when clicking on the icon, nothing happens. I believe this is an issue with material-ui
    • No specific steps to reproduce, but you can be "too accurate" when clicking, and actually click the nested path component, which doesn't appear to trigger the onClick event
      image

Next steps (not covered in this PR)

  • Support naming tabs
  • Confirmation modal before deleting tab

@JonathanGin52 JonathanGin52 marked this pull request as ready for review May 24, 2020 04:25
@elxris
Copy link
Owner

elxris commented May 25, 2020

I will review it, and let you know. Thank you a lot!

@elxris
Copy link
Owner

elxris commented May 30, 2020

Ok, so I was going through this and some thoughts I collected.

Far from design, I think the following only applies to UX, delete button shouldn't be that close to the tab, and not without a warning that all your data is going to be deleted.

Stretch goals:

  • Maybe let the users rename the tabs?
  • Reorder tabs

I also think this an "advanced" feature and we should have like a small panel to enable this features and more to come.

We should have special care for users with data, we can't just delete or not "migrate" their old data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[REQUEST] Support tracking multiple sets of turnip prices
2 participants
@JonathanGin52 @elxris