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

docs: fix withLiveEdits sticky section behaviour for actions #2546

Conversation

malavshah9
Copy link
Contributor

@malavshah9 malavshah9 commented Oct 16, 2024

Resolves #2513
image
image

@YossiSaadi
Copy link
Contributor

Hey @malavshah9, I'm sorry if the issue wasn't clear enough, I meant it should always be visible and stick on the right bottom side. From the very beginning and even when scrolled (middle or bottom of scroll)

@malavshah9
Copy link
Contributor Author

@YossiSaadi Updated PR with sticky behavior.

Thanks!

@malavshah9 malavshah9 changed the title fixes: s sticky section behaviour for actions fixes: withLiveEdits sticky section behaviour for actions Oct 18, 2024
@talkor talkor changed the title fixes: withLiveEdits sticky section behaviour for actions docs: fix withLiveEdits sticky section behaviour for actions Oct 20, 2024
Copy link
Contributor

@YossiSaadi YossiSaadi left a comment

Choose a reason for hiding this comment

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

Amazing @malavshah9, very elegant solution!

I had a few comments :)

  • I think the max-height trick for cm-editor, alongside the position: relative, should get the job done, without rendering children for LiveEditor
  • And maybe even the position: relative can also be removed!

mind making sure? :)

@@ -18,7 +18,8 @@
}

& :global(.cm-editor) {
padding: var(--sb-spacing-medium);
padding: 0 0 0 var(--sb-spacing-medium);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please change that one to

Suggested change
padding: 0 0 0 var(--sb-spacing-medium);
padding: var(--sb-spacing-medium);
padding-right: 0;

we want the rest of the padding, only the right padding is now an issue (because of the scrollbar)

@@ -18,7 +18,8 @@
}

& :global(.cm-editor) {
padding: var(--sb-spacing-medium);
padding: 0 0 0 var(--sb-spacing-medium);
max-height: 500px;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 💯

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can give up on those changes
The actions can be rendered as a sibling and it should still work I believe

@@ -6,4 +6,5 @@
right: 0;
border-top-left-radius: 4px;
overflow: hidden;
z-index: 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't insert the LiveEditor changes for actionComp, this also might be redundant, as the actions would be after the cm-editor in the DOM

Comment on lines 53 to 63
actionComp={
<>
<section className={styles.actions}>
<LiveEditorAction onClick={onCopyClick} disabled={isCopied}>
{isCopied ? "Copied" : "Copy"}
</LiveEditorAction>
<LiveEditorAction onClick={onFormatClick}>Format</LiveEditorAction>
<LiveEditorAction onClick={onResetClick}>Reset</LiveEditorAction>
</section>
</>
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you restore this one to be a sibling rather than as a prop for LiveEditor?

Comment on lines 54 to 60
<section className={styles.actions}>
<LiveEditorAction onClick={onCopyClick} disabled={isCopied}>
{isCopied ? "Copied" : "Copy"}
</LiveEditorAction>
<LiveEditorAction onClick={onFormatClick}>Format</LiveEditorAction>
<LiveEditorAction onClick={onResetClick}>Reset</LiveEditorAction>
</section>
Copy link
Contributor

Choose a reason for hiding this comment

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

to this one

@malavshah9
Copy link
Contributor Author

@YossiSaadi All required changes are done from my side, you can re-review now.

@malavshah9 malavshah9 requested a review from YossiSaadi October 20, 2024 11:29
Copy link
Contributor

@YossiSaadi YossiSaadi left a comment

Choose a reason for hiding this comment

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

Amazing work @malavshah9, thanks for your contribution to Vibe!!

@YossiSaadi YossiSaadi merged commit 4ef4932 into mondaycom:master Oct 20, 2024
4 checks passed
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.

Fix withLiveEdit's sticky section behavior for actions
2 participants