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: floating uninstall button #18047

Closed

Conversation

Justicea83
Copy link
Contributor

@Justicea83 Justicea83 commented Oct 17, 2023

Problem

#18029

Changes

  1. Remove default relative position for Lemon buttons

How did you test this code?

In the browser

Screenshot 2023-10-17 at 14 46 49
  1. Navigate to Browse Apps > Apps Management
  2. Shrink the screen to mobile size
  3. Click on the humburger icon to reveal the sidebar
  4. Ensure the Uninstall button isn't floating around.

@Justicea83 Justicea83 changed the title bug: fix for floating uninstall button bug(floating button): fix for floating uninstall button Oct 17, 2023
@Justicea83 Justicea83 changed the title bug(floating button): fix for floating uninstall button fix(floating button): fix for floating uninstall button Oct 17, 2023
@Justicea83
Copy link
Contributor Author

@raquelmsmith can you help with the merging of this PR, if you like the changes?

@Justicea83
Copy link
Contributor Author

@neilkakkar not sure if you can help me assign a reviewer.

I also want to ask, it there a process for asking for review in this repo.
Check the contributing doc, didn't find such info

@neilkakkar neilkakkar requested review from tiina303 and a team October 20, 2023 11:12
@raquelmsmith
Copy link
Member

this should get thorough front-end review before it is merged as we have lots of buttons all over the place.

@raquelmsmith raquelmsmith changed the title fix(floating button): fix for floating uninstall button fix: floating uninstall button Oct 20, 2023
@raquelmsmith raquelmsmith self-requested a review October 20, 2023 16:08
@Justicea83
Copy link
Contributor Author

Alright sure, maybe we need a secondary testing organization like Applause, to speed this up.

@Justicea83
Copy link
Contributor Author

Meanwhile I will take a look at something else.

Thanks @raquelmsmith

@raquelmsmith
Copy link
Member

maybe we need a secondary testing organization like Applause, to speed this up.

we have lots of visual regression tests so I'm not that concerned, just want to take a manual poke around to be extra sure :) will do that on monday!

@tiina303 tiina303 requested review from Twixes and removed request for a team and tiina303 October 23, 2023 16:08
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this!

This is not the fix though. We need position: relative on .LemonButton e.g. for position of actions in LemonButtonWithSideAction. This also doesn't seem to actually fix the issue.

The real fix is removing the .Plugins__Popconfirm class, which has a problematic z-index that we no longer need.

@raquelmsmith raquelmsmith removed their request for review October 23, 2023 19:59
@Twixes
Copy link
Member

Twixes commented Oct 25, 2023

Hmm, I believe something went wrong when rebasing/merging, because there are many commits considered part of the diff, which already are in master

@Justicea83 Justicea83 force-pushed the fix-floating-uninstall-button branch from 6cbc858 to 95060d6 Compare October 25, 2023 11:49
@Justicea83
Copy link
Contributor Author

This PR is messed up, I'll close it out and open another one

@Justicea83 Justicea83 closed this Oct 25, 2023
@Justicea83 Justicea83 force-pushed the fix-floating-uninstall-button branch from 95060d6 to 46c0750 Compare October 25, 2023 11:54
@posthog-deployer posthog-deployer bot temporarily deployed to dev October 25, 2023 11:56 Destroyed
@posthog-deployer posthog-deployer bot temporarily deployed to prod October 25, 2023 12:02 Destroyed
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.

3 participants