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

web: Add a resize drag component to the sidebar #4774

Merged
merged 3 commits into from Jul 21, 2021
Merged

web: Add a resize drag component to the sidebar #4774

merged 3 commits into from Jul 21, 2021

Conversation

nicks
Copy link
Member

@nicks nicks commented Jul 19, 2021

Hello @lizzthabet,

Please review the following commits I made in branch nicks/ch12138:

5b89976 (2021-07-19 13:50:04 -0400)
web: Add a resize drag component to the sidebar
Fixes #4614

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks nicks requested a review from lizzthabet July 19, 2021 17:51
@shortcut-integration
Copy link

This pull request has been linked to Clubhouse Story #12138: better support for long resource names.

@nicks
Copy link
Member Author

nicks commented Jul 19, 2021 •
edited

cc @SurbhiSGupta this is mostly a lightweight experiment to see how it feels. The drag component doesn't currently take up any width. It's only noticable if you mouse over and your cursor changes.

@nicks
Copy link
Member Author

nicks commented Jul 19, 2021

also: there's no built-in MUI resizer component, but there's a lot of discussion about adding one, and they pointed to this component as the inspiration (which is why i picked it)

@milas
Copy link
Contributor

milas commented Jul 19, 2021

Ooo I'm excited for this, I have instinctively tried to resize the sidebar countless times 😅

lizzthabet
lizzthabet approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@lizzthabet lizzthabet left a comment

Choose a reason for hiding this comment

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

this works beautifully! i only have two optional comments.

web/src/OverviewResourceSidebar.tsx Outdated
height: 100%;
width: 336px;
min-width: 336px;
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) since we're using this value in a couple places/files, could you extract into a const?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

web/src/OverviewResourcePane.tsx Outdated
alerts={alerts}
buttons={buttons}
/>
<SplitPane split="vertical" minSize={336} defaultSize={336}>
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about adding the split pane to the storybook component for OverviewResourceSidebar? it might be nice to have that as part of the demo view.

Copy link
Member Author

Choose a reason for hiding this comment

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

ooh i like that idea!

@nicks nicks merged commit 0b7ba49 into master Jul 21, 2021
@nicks nicks deleted the nicks/ch12138 branch July 21, 2021 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizzthabet lizzthabet lizzthabet approved these changes

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

Successfully merging this pull request may close these issues.

better support for long resource names
3 participants
@nicks @milas @lizzthabet