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

[fix] Fix persistence of chart scale selection #164

Merged
merged 11 commits into from
May 21, 2024
Merged

Conversation

2Steaks
Copy link
Collaborator

@2Steaks 2Steaks commented Feb 5, 2024

Closes #153

What?

This PR fixes an issue with the time range selection persisting upon data refresh.

Note

This does NOT fix active chart selection: #154

Why?

The time range selection would reset with every data update.

Additional context

  • Add global time range context
  • Update chart hook to handle scale selection
Screen.Recording.2024-02-05.at.10.50.33.mov

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (mage lint) and all checks pass.
  • I have run tests locally (mage test) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

- Add global time range context
- Update chart hook to handle scale selection
@2Steaks 2Steaks requested a review from szkiba February 5, 2024 10:55
@2Steaks 2Steaks self-assigned this Feb 5, 2024
Copy link
Collaborator

@szkiba szkiba left a comment

Choose a reason for hiding this comment

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

It works perfectly. (I ran an asset generation and pushed it)
I have only one comment: I think there should be some visual feedback that a time interval is currently selected and therefore the graphs are not updated. For example, a button in the header: "enable refresh" or similar. Otherwise, the user will not know how to enable it and why it is not updated (especially after switching tabs).

- Add time range reset button component
- Add footer component
- Add rewind-time icon
- Update button styles
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@2Steaks
Copy link
Collaborator Author

2Steaks commented Feb 9, 2024

It works perfectly. (I ran an asset generation and pushed it) I have only one comment: I think there should be some visual feedback that a time interval is currently selected and therefore the graphs are not updated. For example, a button in the header: "enable refresh" or similar. Otherwise, the user will not know how to enable it and why it is not updated (especially after switching tabs).

Screen.Recording.2024-02-09.at.11.41.22.mov

@2Steaks 2Steaks closed this Feb 9, 2024
@2Steaks 2Steaks reopened this Feb 9, 2024
@2Steaks 2Steaks requested a review from szkiba February 9, 2024 11:52
Copy link
Collaborator

@szkiba szkiba left a comment

Choose a reason for hiding this comment

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

@2Steaks That's what I was thinking about the button, thanks.
I also like that it is responsive. Is there a reason why the button is placed at the bottom of the page in the case of a small browser window? At the top of the page, the tabs are placed in a separate row, which would also fit the "reset time range" button at the top of the page.
My other comment is that the "reset time range" button should not be displayed in the summary tab, because nothing there depends on the selected time frame. In general, if there is no chart on the page, this button should not be used. This would visually lead to the top row "jumping", because a button would disappear from the middle. Perhaps this button could be placed in front of the icons (with an appropriate design), and then it wouldn't be disturbing if it disappeared from time to time. The other option is that it will be disabled if there is no chart on the page.

Copy link
Collaborator

@szkiba szkiba left a comment

Choose a reason for hiding this comment

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

The "reset time range" button should not be displayed in the summary tab, because nothing there depends on the selected time frame. In general, if there is no chart on the page, this button should not be used. This would visually lead to the top row "jumping", because a button would disappear from the middle. Perhaps this button could be placed in front of the icons (with an appropriate design), and then it wouldn't be disturbing if it disappeared from time to time. The other option is that it will be disabled if there is no chart on the page.

# Conflicts:
#	dashboard/assets/packages/ui/dist/assets/index-4796c824.js
#	dashboard/assets/packages/ui/dist/assets/index-5a43f090.js
#	dashboard/assets/packages/ui/dist/assets/index-d358b692.js
#	dashboard/assets/packages/ui/dist/index.html
I have switched the collapsing order for mobile so that the nav items stay next to the logo, and the actions (reset, report, etc..) drop to the next row.
@2Steaks 2Steaks requested a review from a team as a code owner May 20, 2024 10:55
@2Steaks 2Steaks requested review from pablochacin and szkiba and removed request for a team May 20, 2024 10:55
@2Steaks
Copy link
Collaborator Author

2Steaks commented May 20, 2024

The "reset time range" button should not be displayed in the summary tab, because nothing there depends on the selected time frame. In general, if there is no chart on the page, this button should not be used. This would visually lead to the top row "jumping", because a button would disappear from the middle. Perhaps this button could be placed in front of the icons (with an appropriate design), and then it wouldn't be disturbing if it disappeared from time to time. The other option is that it will be disabled if there is no chart on the page.

UI updated:

example.mov

Copy link
Collaborator

@szkiba szkiba left a comment

Choose a reason for hiding this comment

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

LGTM

@szkiba szkiba removed the request for review from pablochacin May 21, 2024 12:16
@szkiba szkiba merged commit e977608 into master May 21, 2024
10 checks passed
@szkiba szkiba deleted the fix/chart-refresh-state branch May 21, 2024 13:56
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.

Selected timeframe resets on next data refresh
2 participants