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

Redesign panel sections in sidebar #1704

Merged
merged 14 commits into from
Nov 14, 2023
Merged

Redesign panel sections in sidebar #1704

merged 14 commits into from
Nov 14, 2023

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Sep 29, 2023 •
edited
Loading

Description of proposed changes

Hopefully a good improvement to user experience.

Related issue(s)

Checklist

  • Add back behavior described in dd37b3f
  • Add entropy toggle
  • Finalize design
  • Update info tooltips (thread)
  • Make issue for modifying panelsAvailable when showTreeToo is changed (thread)
  • Make issue for how to define types for prop drilling (thread)
  • Fix alignment of toggle with panel title (thread)
  • Update changelog
  • Checks pass
  • (to be done by a Nextstrain team member) Create preview PRs on downstream repositories.
    • unselectedBackground still works

@victorlin victorlin self-assigned this Sep 29, 2023
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-move--kkdqqa September 29, 2023 20:36 Inactive
@victorlin
Copy link
Member Author

@jameshadfield this PR is unfinished but would like to get your opinion on general implementation direction.

All the remaining tasks that I can think of are in the PR description - please add anything that I might be missing!

jameshadfield
jameshadfield requested changes Oct 4, 2023
View reviewed changes
src/components/controls/controls.tsx Outdated Show resolved Hide resolved
src/components/controls/controls.tsx Outdated Show resolved Hide resolved
src/components/controls/panel-toggle.tsx Outdated Show resolved Hide resolved
src/components/controls/controls.tsx Outdated Show resolved Hide resolved
@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch 3 times, most recently from 82015f3 to 16e680b Compare October 5, 2023 20:08
This was referenced Oct 9, 2023
Closed
Closed
@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch from 16e680b to 9838191 Compare October 12, 2023 19:20
@victorlin victorlin changed the base branch from master to victorlin/use-modern-redux October 12, 2023 19:20
Base automatically changed from victorlin/use-modern-redux to master October 12, 2023 22:24
@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch 2 times, most recently from 891ee1a to 705b64c Compare October 19, 2023 19:40
@nextstrain-bot nextstrain-bot mentioned this pull request Oct 19, 2023
Closed
@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch from 705b64c to c9e4480 Compare October 19, 2023 21:10
jameshadfield
jameshadfield requested changes Oct 20, 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 Victor. Overall some nice reorganisations here. Most (all?) of my comments are minor.

src/components/controls/controlHeader.tsx Show resolved Hide resolved
src/components/controls/annotatedTitle.tsx Outdated Show resolved Hide resolved
src/components/controls/controls.tsx Outdated Show resolved Hide resolved
src/components/controls/panelSection.tsx Outdated Show resolved Hide resolved
src/components/controls/panelHeader.tsx Outdated Show resolved Hide resolved
src/components/controls/panelChevron.tsx Show resolved Hide resolved
src/components/controls/panelHeader.tsx Outdated Show resolved Hide resolved
src/components/controls/panelToggle.tsx Outdated Show resolved Hide resolved
src/components/controls/styles.js Outdated Show resolved Hide resolved
src/components/controls/controls.tsx Show resolved Hide resolved
@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch 4 times, most recently from f080bcb to a69a067 Compare October 23, 2023 20:56
@victorlin victorlin mentioned this pull request Oct 23, 2023
Merged
3 tasks
@victorlin
Copy link
Member Author

@jameshadfield it's probably hard to track in the sea of notifications/threads. Here's links to places where I'd like your input, as of now:

jameshadfield
jameshadfield reviewed Nov 8, 2023
View reviewed changes
src/components/controls/controlHeader.tsx Outdated Show resolved Hide resolved
src/components/controls/controlHeader.tsx Show resolved Hide resolved
src/components/controls/miscInfoText.js Outdated Show resolved Hide resolved
@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch 2 times, most recently from 1864c9c to ea97343 Compare November 9, 2023 00:06
@victorlin
PanelToggles: Add context to implicit behavior
82d3883
@victorlin
AnnotatedHeader: Convert to TypeScript
1dd9bd8 
The component was written to allow tooltip as optional, so make that
explicit. Other props should be required.
@victorlin
AnnotatedHeader: Use separate styled component for font-related styles
46ed345 
This keeps the styling closer to where it is used.
@victorlin
Make Language its own controls section
620bfed 
Previously, it looked like language was a part of Panel Options.
@victorlin
Determine panel visibility states in controls component
a0ebda5 
Instead of determining in the sidebar and passing down as props.
@victorlin
tsconfig: Disable exactOptionalPropertyTypes
8a96a09 
Disabling this makes typed prop drilling more readable by not requiring
a conditional for optional props¹.

¹ https://stackoverflow.com/a/48816255
@victorlin
Prepare for panel-specific header changes
ce518c5 
Split AnnotatedHeader into AnnotatedTitle and ControlHeader. A future
commit will introduce PanelHeader.

This places the header info icon immediately to the right of the title,
instead of the far right side (to make room for the upcoming panel
visibility toggle).
@victorlin
Copy link
Member Author

Force-pushing a rebase onto latest master.

@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch 2 times, most recently from b25e783 to a8f8b0d Compare November 10, 2023 01:10
@victorlin
Move panel toggles to control headers
e441dae 
Do this with some new panel-specific components that build upon the
existing AnnotatedTitle and Toggle components.

Simplify headers and translations to just the panel name (e.g. Tree)
instead of "Show <panel name>" / "<panel name> Options".

Conditionally render the entire "Panel Options" section including the
header.

This also adds a section for the entropy panel to maintain its
toggle-ability. This is an empty section because it does not have any
sidebar options.
@victorlin
Rename unselectedBackground to alternateBackground
1ad4376 
This color is now used for more than just the unselected background.
@victorlin victorlin mentioned this pull request Nov 13, 2023
Closed
@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch from a8f8b0d to e3eabae Compare November 13, 2023 21:58
@victorlin
Rename "Panel Options" to "Display"
404e2d4 
I chose the title "Display" even though it is called "layout" in the
state because "Layout" is already used to describe tree layout, and I
didn't want translations to conflict.

I also contemplated putting this control at the top. However, its
conditional rendering worsens usability upon toggling panel visibility,
as the scroll position is changed due to its placement above the
toggles.
@victorlin
Vertically align items in HeaderContainer
e117e19
@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch from e3eabae to 8a69ca2 Compare November 13, 2023 22:45
@victorlin victorlin temporarily deployed to auspice-victorlin-move--smhs4h November 13, 2023 22:46 Inactive
@victorlin
Copy link
Member Author

Force-pushed two more design touch-ups:

  1. Move full/grid display control back to the bottom, since its conditional rendering when placed at the top worsens usability (noted in 404e2d4).
  2. Prevent the option-less Entropy section from showing a pointer on hover.

jameshadfield
jameshadfield approved these changes Nov 14, 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.

I haven't re-reviewed the code changes, but UI-3 is working well and, judging by the commentary on slack, looks good to merge!

@victorlin victorlin force-pushed the victorlin/move-panel-toggles branch from 8a69ca2 to beadef7 Compare November 14, 2023 22:11
This was referenced Nov 14, 2023
Open
Open
@victorlin victorlin marked this pull request as ready for review November 14, 2023 22:23
@victorlin victorlin merged commit 4e2de85 into master Nov 14, 2023
17 checks passed
@victorlin victorlin deleted the victorlin/move-panel-toggles branch November 14, 2023 22:33
@victorlin victorlin mentioned this pull request Nov 15, 2023
Draft
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joverlee521 joverlee521 joverlee521 left review comments

@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.
4 participants
@victorlin @jameshadfield @joverlee521 @nextstrain-bot