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

Tweak update button position and visibility in component editor #673

Merged
merged 5 commits into from
Jul 25, 2022

Conversation

bytasv
Copy link
Contributor

@bytasv bytasv commented Jul 20, 2022

image

  • Moved toolbar below the editor
  • Also changed variant for button so it's more visible. IMO since its the main CTA it could use that extra visibility
  • Fix split-pane height issue (seems that because of its using absolute positioning (weird) it breaks when immediate parent doesn't have relative or any other position set). FYI I've addded fix to packages/toolpad-app/src/components/SplitPane.tsx as to me it seems more intuitive to work like that everywhere. I've also checked other places where SplitPane is used and it seems the rest is still working as expected

Closes #613

@render
Copy link

render bot commented Jul 20, 2022

@oliviertassinari oliviertassinari requested a deployment to move-component-update-button - toolpad-db PR #673 July 20, 2022 14:03 — with Render Abandoned
@bytasv bytasv requested a review from Janpot July 20, 2022 14:03
@bytasv bytasv force-pushed the move-component-update-button branch from 8d12879 to 223fffd Compare July 21, 2022 10:55
@bytasv bytasv force-pushed the move-component-update-button branch from 223fffd to dd0a79a Compare July 21, 2022 11:03
@bytasv
Copy link
Contributor Author

bytasv commented Jul 21, 2022

@Janpot when you get a chance please re-review, I've changed approach and addressed overscroll issue

@@ -38,6 +38,9 @@ const WrappedSplitPane = React.forwardRef<
// Some sensible defaults
minSize={20}
maxSize={-20}
style={{
position: 'relative',
Copy link
Member

@Janpot Janpot Jul 21, 2022

Choose a reason for hiding this comment

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

Yep, makes sense. Perhaps it could make a bit more sense on the paneStyle instead, since that's the exact rectangle we want that editor to size to? (Not 100% sure that will work, didn't test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial though too, I tried it but it seems that paneStyle is responsible for other components so I had to use style

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so it's the whole splitpane sizing badly. I'm a bit concerned that this will mess up existing splitpane usage. Perhaps it's safer to keep it absolute and have it size against its parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked and we have 5 places where we use it:

  1. Component editor - all seems to look ok ✅
  2. Page editor - it's rendered also without an issue ✅

image

  1. Three places in QueryEditor - all seem to look still ok ✅

image

Should I still change it to absolute? I'm worried that having it as absolute will bite us back in future usecases the same way and we will either have to sprinkle relative position on parent everywhere we use SplitPane or we will still end up doing it globally 🤔

@Janpot
Copy link
Member

Janpot commented Jul 21, 2022

Screenshot 2022-07-21 at 20 14 00

👌 horizontal scrollbars

@render
Copy link

render bot commented Jul 25, 2022

@oliviertassinari oliviertassinari requested a deployment to move-component-update-button - toolpad-db PR #673 July 25, 2022 08:25 — with Render Abandoned
@bytasv bytasv merged commit 3021c8c into master Jul 25, 2022
@bytasv bytasv deleted the move-component-update-button branch July 25, 2022 08:53
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.

In custom component interface, move 'Update' button to the bottom right.
3 participants