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

Relocate the details toggle #15117

Merged
merged 10 commits into from
Jun 3, 2024
Merged

Relocate the details toggle #15117

merged 10 commits into from
Jun 3, 2024

Conversation

brianjhanson
Copy link
Contributor

Relocates the details sidebar toggle button to the side of the content editor.

CleanShot.2024-05-31.at.10.59.46.mp4

@brianjhanson brianjhanson requested review from i-just, brandonkelly and gcamacho079 and removed request for i-just May 31, 2024 16:00
gcamacho079
gcamacho079 requested changes May 31, 2024
View reviewed changes
Copy link
Contributor

@gcamacho079 gcamacho079 left a comment

Choose a reason for hiding this comment

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

😍 Looks awesome. The only thing I noticed is that at 320px wide viewports, the .content-pane overflows the body element.

@brandonkelly
Copy link
Member

Looks great!

The hairline is 4px closer to the sidebar pane than the content pane, so that needs to be centered.

Ideally, both sides can be tightened up, so the distance between the two panes is 24px, to match the distance between the global sidebar and the content pane. So shave off 7px from the left side of the hairline, and 3px from the right side, I think.

@brianjhanson brianjhanson requested a review from gcamacho079 June 3, 2024 15:11
@brianjhanson
Copy link
Contributor Author

brianjhanson commented Jun 3, 2024 •
edited
Loading

Looks great!

The hairline is 4px closer to the sidebar pane than the content pane, so that needs to be centered.

Ideally, both sides can be tightened up, so the distance between the two panes is 24px, to match the distance between the global sidebar and the content pane. So shave off 7px from the left side of the hairline, and 3px from the right side, I think.

This difference was intentional to try to make a stronger association between the sidebar and the toggle, but I think you're right and the contrast between the spacing wasn't quite enough to make that obvious, so I've centered it now.

@brandonkelly
Copy link
Member

Thanks, looks great now!

@brandonkelly
Copy link
Member

@brianjhanson One more bug: for RTL languages, the icon needs to be inverted:

screenshot of the new sidebar toggle, on a right-to-left interface orientation

@brandonkelly brandonkelly merged commit 340a67e into 5.2 Jun 3, 2024
@brandonkelly brandonkelly deleted the feature/move-details-toggle branch June 3, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonkelly brandonkelly Awaiting requested review from brandonkelly

@gcamacho079 gcamacho079 Awaiting requested review from gcamacho079

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brianjhanson @brandonkelly @gcamacho079