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

Move mobile display detection to Redux #1717

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Move mobile display detection to Redux #1717

merged 4 commits into from
Nov 10, 2023

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Oct 23, 2023 •
edited
Loading

Description of proposed changes

Addressing a task defined in a comment:

auspice/src/components/main/index.js

Lines 53 to 54 in ba7e1c2

as state here, but his has since ben moved to redux state. The mobile
display should likewise be lifted to redux state */

and getting rid of the prop drilling before more layers are added in #1704.

Related issue(s)

Testing

  • Checks pass
  • There should be no functional changes, and it'd be good to check that all modified components still behave properly.
    • Confirm existing behavior: Interface on a window with narrow width defaults to hidden sidebar, and when the sidebar is opened, there is no info icon for hover. Tested locally on Chromium and Safari.

@victorlin
Move mobile display detection to Redux
6644454 
Previously, it was determined in the Main component and passed down as
props.

It is still passed down as props, but that is no longer necessary and
subsequent commits will remove that.

I chose state.general.mobileDisplay since this seems like a "general"
value.
@victorlin
Controls: Remove prop drilling of mobileDisplay
700f262 
It is now available in the Redux state, so access it directly within
AnnotatedHeader and ChooseExplodeAttr.
@victorlin
Remove unused mobileDisplay prop
de36e48 
This was already unused in NavBar, and it is newly unused in Sidebar.
@victorlin
Sidebar: Access typed controls state within component
f965ddf 
Instead of passing down from Main.

Also take the opportunity to convert this file to TypeScript.
@victorlin victorlin self-assigned this Oct 23, 2023
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-move--ralwm0 October 23, 2023 22:51 Inactive
@jameshadfield jameshadfield mentioned this pull request Nov 8, 2023
Merged
15 tasks
jameshadfield
jameshadfield approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Thanks!

@victorlin victorlin merged commit 477d27b into master Nov 10, 2023
19 checks passed
@victorlin victorlin deleted the victorlin/move-mobileDisplay-to-redux branch November 10, 2023 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameshadfield jameshadfield jameshadfield approved these changes

Assignees

@victorlin victorlin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@victorlin @jameshadfield @nextstrain-bot