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

fix: modals positions once again #5163

Merged
merged 1 commit into from May 12, 2022

Conversation

yshwaker
Copy link
Contributor

fixes #5162

Getting the width of editor from textarea element is not reliable sometimes, especially when there are code blocks in it.

This pr tries to fix the position by getting the width from .editor-wrapper. iirc, the element with this class represent the current active block area and it seems to be unique which should be safe to get the editor width(I Hope).

@llcc llcc self-requested a review April 30, 2022 09:23
Copy link
Contributor

@llcc llcc left a comment

Choose a reason for hiding this comment

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

Thanks @yshwaker. Glad that you fixed this issue. Great!

Copy link
Collaborator

@cnrpman cnrpman left a comment

Choose a reason for hiding this comment

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

LGTM

@logseq-cldwalker
Copy link
Collaborator

Sorry. I merged in #5193 and didn't see that this PR also solves a similar issue. Does this PR fix any other bugs that the other PR doesn't solve? If not, I'm be inclined to stick with that solution

@yshwaker
Copy link
Contributor Author

yshwaker commented May 6, 2022

@logseq-cldwalker it fixed the same issue, but i'm not sure if the added ls-textarea is necessary. they are both quick fix, but ultimately i feel using query selector to get a dom element in React is kind of weird since it makes the modal component not self contained. it seems the right way to do is passing a ref prop to the absolute-modal, and have some safety check in it.

@pengx17
Copy link
Contributor

pengx17 commented May 6, 2022

Oops, sorry I didn't notice that you created this PR earlier and I agree your solution is cleaner than mine.

@yshwaker
Copy link
Contributor Author

yshwaker commented May 6, 2022

@pengx17 no worries.

@logseq-cldwalker
Copy link
Collaborator

@yshwaker Sounds good. If you can update the conflict, we can test it again

@yshwaker
Copy link
Contributor Author

@logseq-cldwalker sorry for the late reply. just resolved the conflict.

Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

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

@yshwaker Works great! Thanks for the fix 🐛 ⚡

@logseq-cldwalker logseq-cldwalker merged commit 22d4166 into logseq:master May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dropdown position incorrect
5 participants