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 Windows bug with the admin file upload widget #21687

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

cathysarisky
Copy link
Contributor

@cathysarisky cathysarisky commented Nov 21, 2024

closes #21439

On Windows 10/Chrome (but maybe nowhere else?), attempting to drag a file into any of the drop targets in the admin panel resulted in flickering behavior, and generally dropping didn't actually trigger the upload.

I thought originally it was a problem with the size of the drop target, but it actually appears to be a rerender bug. In brief, handleDragging and handleStopDragging were firing repeatedly, and each fire triggered a rerender, that added or removed a div from the file upload widget.

I suspect some browser-specific difference in how drag events fire is to blame.

This PR moves the logic to change the classes applied to the div, rather than changing whether the div is present.

I have manually tested with Windows 10 in the users import, theme import, and content import widgets. Styles are preserved (although I think they could be improved, as the grey outline is really faint) and uploading now works consistently, instead of mostly triggering display of the raw file most of the time.

@cathysarisky cathysarisky changed the title fix upload widget render/drag bug 🐛 fix Windows bug with the admin file upload widget Nov 21, 2024
@9larsons
Copy link
Contributor

LGTM. I don't notice a diff on Mac browsers and the performance improvements are a nice bonus. Thanks!

@9larsons 9larsons merged commit ebd98d0 into TryGhost:main Nov 22, 2024
18 checks passed
ronaldlangeveld added a commit that referenced this pull request Dec 16, 2024
ref ENG-1809

- Changing the z-index of the overlay ensure the button can be hovered
  with the cursor as expected which was broken by #21687
- need to ensure that this doesn't undo the initial "fix" which affects
  windows users
@ronaldlangeveld
Copy link
Member

Hey @cathysarisky @vikaspotluri123 👋

First off, thank you for the initial fix—it addressed the core issue well!

However, we noticed a side effect that impacted the button’s hover functionality and caused some Admin-X Settings tests to fail. The issues are directly related to this fix. 🙈

Expected behaviour:

Screen.Recording.2024-12-16.at.5.05.31.pm.mov

Current side effect:

Screen.Recording.2024-12-16.at.5.06.34.pm.mov

Upon investigation, it seems the <div> element used for the fix is positioned on top of the actual button, which is likely causing the hover issue.

To address this, I made a small adjustment by adding a z-index to the <div> element. This fix resolves the broken tests and restores the expected hover behaviour.
Here’s the PR for reference:
Proposed Fix

Unfortunately, I don’t currently have access to a Windows machine to verify that this change doesn’t undo the original fix. Would you mind taking a look and testing it on Windows?

Let me know your thoughts or if there’s anything else you’d like me to adjust.

Thanks again for your help! 🙏🏻

cc @9larsons

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.

Drop target for uploads (content and theme) is broken.
4 participants