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

Fix always open menu on article sidebar after selection #615

Merged
merged 3 commits into from
Nov 18, 2023
Merged

Fix always open menu on article sidebar after selection #615

merged 3 commits into from
Nov 18, 2023

Conversation

seog-jun
Copy link
Contributor

@seog-jun seog-jun commented Nov 5, 2023

✨ Codu Pull Request 💻

Codu Logo

👉 Please remove the below and replace with your own values, leaving the headers where they are. 👈

Pull Request details:

Fixes #594
Fixed the issue that Popover.panel always opens when Report Article is clicked.

Any Breaking changes:

None

Associated Screenshots:

Popover.1.mp4

p.s: Let me know if there's something not working or I'm missing, Thanks!

@vercel Vercel
Copy link

vercel bot commented Nov 5, 2023

@snyk-bot is attempting to deploy a commit to the Codú Team on Vercel.

A member of the Team first needs to authorize it.

@vercel Vercel
Copy link

vercel bot commented Nov 6, 2023 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
codu ❌ Failed (Inspect) Nov 15, 2023 9:30pm

NiallJoeMaher
NiallJoeMaher requested changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@NiallJoeMaher NiallJoeMaher left a comment

Choose a reason for hiding this comment

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

Thanks for this! Only one question/suggestion. 🦾

components/ArticleMenu/ArticleMenu.tsx
@@ -164,7 +176,10 @@ const ArticleMenu = ({
leaveFrom="transform opacity-100 scale-100"
leaveTo="transform opacity-0 scale-95"
>
<Popover.Panel className="origin-top-right absolute bottom-14 right-0 lg:left-16 lg:bottom-0 mt-2 w-48 rounded-md shadow-lg py-1 bg-white dark:bg-white ring-1 px-1 ring-black ring-opacity-5 focus:outline-none">
<Popover.Panel
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than styling, do you think this might be a cleaner solution? https://headlessui.com/react/popover#closing-popovers-manually This lets you use the internal popover state to close things.

Let me know what you think!

@seog-jun
Copy link
Contributor Author

@NiallJoeMaher Hi, let me know if there are more questions/suggestions about it. Just for a reminder, this PR is still not merged yet.

@NiallJoeMaher
Copy link
Contributor

@NiallJoeMaher Hi, let me know if there are more questions/suggestions about it. Just for a reminder, this PR is still not merged yet.

Hey! Yes, I added a change request but didn't see any comments or updates, so I was waiting for that. 🦾 Let me know if I can help with anything.

@seog-jun
Copy link
Contributor Author

seog-jun commented Nov 11, 2023 •
edited

@NiallJoeMaher Hi, let me know if there are more questions/suggestions about it. Just for a reminder, this PR is still not merged yet.

Hey! Yes, I added a change request but didn't see any comments or updates, so I was waiting for that. 🦾 Let me know if I can help with anything.

Oh I left comment under your reply, but it was pending, sorry about that! And this is my reply:

@NiallJoeMaher
Thanks for your feedback! I also thought this would be a clearer solution and I already approached this way However, when I use Popover.Button with ReportModal component, ReportModal component shows up and disappears in a second because Popover.Button closes the the whole Popover.Panel . Maybe I'm wrong, but it didn't really work for me, that's the reason why I used styling as a workaround.

@NiallJoeMaher
Copy link
Contributor

@NiallJoeMaher Hi, let me know if there are more questions/suggestions about it. Just for a reminder, this PR is still not merged yet.

Hey! Yes, I added a change request but didn't see any comments or updates, so I was waiting for that. 🦾 Let me know if I can help with anything.

Oh I left comment under your reply, but it was pending, sorry about that! And this is my reply:

@NiallJoeMaher Thanks for your feedback! I also thought this would be a clearer solution and I already approached this way However, when I use Popover.Button with ReportModal component, ReportModal component shows up and disappears in a second because Popover.Button closes the the whole Popover.Panel . Maybe I'm wrong, but it didn't really work for me, that's the reason why I used styling as a workaround.

Perfect let me test it and get back to you! If it isn't work as I hoped I'll get this merged. Thanks!

NiallJoeMaher
NiallJoeMaher approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@NiallJoeMaher NiallJoeMaher left a comment

Choose a reason for hiding this comment

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

LGTM! I'm not sure why the build is breaking but we will get there. 😢

@seog-jun
Copy link
Contributor Author

@NiallJoeMaher Thanks for the review! Let me know if there's something that I have to fix for the build.

@NiallJoeMaher NiallJoeMaher merged commit d175849 into codu-code:develop Nov 18, 2023
3 checks passed
@NiallJoeMaher
Copy link
Contributor

Sorry about the delay! I had a nightmare over the last few days with my AWS pipeline! Thanks for the work. 🦾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NiallJoeMaher NiallJoeMaher NiallJoeMaher approved these changes

@pkspyder007 pkspyder007 Awaiting requested review from pkspyder007

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

Successfully merging this pull request may close these issues.

Fix always open menu on article sidebar after selection
3 participants
@seog-jun @NiallJoeMaher @snyk-bot