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 drag and drop area for file choosers by adding separate ones #2368

Merged
merged 14 commits into from
Dec 2, 2024

Conversation

omar-ahmed42
Copy link
Contributor

Description

  • Add separate drag and drop area for file choosers.

Why?

Previously, when there were multiple file choosers in the same page, if you attempted to drag and drop any files, they would be added to both file choosers as it was designed at first to handle 1 file chooser present, now that we have multiple ones, it is necessary to adapt our design to match the changing functionality.

Can you not preserve the old overlay when there's only one file chooser present?

Yes, we can, but imagine as a user, you try to drag and drop a file in one page and the fields turn into drag and drop areas then you go to another page and try to drag and drop again but you encounter the old overlay instead, as a user you might get confused and ask yourself "What changed?" or if a user is telling another user the steps to drag and drop files and he didn't know about this case, then it would still be confusing, thus consistency is preferred in this case.

UI Change:

Before dragging and dropping:

image

During dragging and dropping:

image

Closes #2367

Checklist

  • I have read the Contribution Guidelines
  • I have performed a self-review of my own code
  • I have attached images of the change if it is UI based
  • I have commented my code, particularly in hard-to-understand areas
  • If my code has heavily changed functionality I have updated relevant docs on Stirling-PDFs doc repo
  • My changes generate no new warnings
  • I have read the section Add New Translation Tags (for new translation tags only)

 - Add separate drag and drop area for file choosers

### Why?
Previously, when there were multiple file choosers in the same page, if you attempted to drag and drop any files, they would be added to both file choosers as it was designed at first to handle 1 file chooser present, now that we have multiple ones, it is necessary to adapt our design to match the changing functionality.

### Can you not preserve the old overlay when there's only one file chooser present?
Yes, we can, but imagine as a user, you try to drag and drop a file in one page and the fields turn into drag and drop areas then you go to another page and try to drag and drop again but you encounter the old overlay instead, as a user you might get confused and ask yourself "What changed?" or if a user is telling another user the steps to drag and drop files and he didn't know about this case, then it would still be confusing, thus consistency is preferred in this case.
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Nov 30, 2024
@github-actions github-actions bot added the Front End Issues or pull requests related to front-end development label Nov 30, 2024
@omar-ahmed42 omar-ahmed42 changed the title Add separate drag and drop area for file choosers Fix drag and drop area for file choosers by adding separate ones Nov 30, 2024
@Frooodle
Copy link
Member

Frooodle commented Dec 1, 2024

I think we should increase the height of the drag drop area maybe 50-100% more

@omar-ahmed42
Copy link
Contributor Author

I agree that the height should increase, but I feel like 25-50% would be the sweet spot compared to 100% as this going above 50% would require UI changes (possibly tons of margin in between elements) so that it doesn't cover other elements (at 25-50% we would still need to add mb-3 to file choosers and in sign page specifically we would need to add another mt-3 to the box below that contains the type of signature (image, text, etc).

Unless we would redesign our file upload in the first place to look something like

image
(this would placed in a box of course with dashed borders)

this way we could have it any size we want.

If you have any ideas, let me know

@Frooodle
Copy link
Member

Frooodle commented Dec 1, 2024

Yeah to be honest i think we could replace the whole file input
have it always be the drag and drop files here box, I think it looks nicer

"Click to upload OR drag and drop" and have it remain as the dotted outlined box version
Probably have it generated a list of files under it which can be manually removed from being selected as well

I think continuing using the Browse inputter is something we should stop

@omar-ahmed42
Copy link
Contributor Author

omar-ahmed42 commented Dec 1, 2024

This is what I went for some far (I might change font colors to go along with the dark/light modes, same goes for the background and the button color, but this is ) (the image is below).

The bottom box should stay hidden unless there are files uploaded.

Each uploaded file should have an icon based on its extension, if it's unknown or we have no icon for it at the moment it should be represented as an empty document icon as in the last icon in the second row.

Each file should have its name displayed (if it's too long then you'll have to hover the name) and the file size (we might convert it to the closest unit, example: files less than 1 MB could be represented as X KBs - where X is the size of the file after conversion - files greater than 1 MB and less than 1 GB would would represented as X MBs and so on).

Why include the name and size and not just "name"? in case more than one file had the same name but different sizes, so that the user can remove the correct one.

Why use a box containing icons and their info rather than have them in a vertical list? Because it would occupy less space, if the user has too many files, that way the user wouldn't have to scroll too much.

Let me know if you have any notes/questions/ideas

image

@Frooodle
Copy link
Member

Frooodle commented Dec 1, 2024

Looks awesome! Will review more tomorrow and merge!

Awesome work as always

@Frooodle
Copy link
Member

Frooodle commented Dec 1, 2024

I'm very happy with the implementation you described and reasoning

@omar-ahmed42
Copy link
Contributor Author

Awesome! Thanks a lot! I will start pushing my changes so that it would be ready by tomorrow hopefully

- Selected files are listed below the file chooser in a selected files container.
- Users can now remove uploaded/selected files.
- Hide selected files container/box unless there are files selected/uploaded.
- Add separate overlay for each drag & drop area.

## FAQ:
- Why did you assign a unique id to each file? isn't the filename enough?
= Because a user might upload multiple files with the same name, if the user wanted to remove one of them, how would we differentiate between them? we won't be able to unless we assign an identifier, you might argue "we remove based on the filename and size", then what if the user uploaded the same file more than once (intentionally), then we would accidentally remove all the files even though that is not what the user wanted, so going with unique ID approach would prevent this issue/problem from occurring in the first place.
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 2, 2024
- Rename remove-file css class to remove-selected-file to avoid css conflict with remove-file in merge.css
Use the correct element to dispatch "file-input-change" (input element is the correct one).
@omar-ahmed42
Copy link
Contributor Author

The only thing remaining for me is to take a look at how themeing (dark/light) works in the project, to make the UI change based on the current theme, other than that, I believe it's ready. (It still works with light theme but it doesn't look good, so we would need to adjust that)

@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

Nice I'll hold off merge and review the current impl

/deploypr

Copy link
Contributor

github-actions bot commented Dec 2, 2024

🚀 PR Test Deployment

Your PR has been deployed for testing!

🔗 Test URL: http://185.252.234.121:2368

This deployment will be automatically cleaned up when the PR is closed.

- Adapt file chooser UI to themes by adjusting their font colors and background colors.
- Make text more visible in overlay by increasing the font size by 0.1rem and setting font weight to 550.
@omar-ahmed42
Copy link
Contributor Author

omar-ahmed42 commented Dec 2, 2024

Finally added support for themeing

Light theme:

image

image

image

Dark theme:

image

image

image

If you encounter any issues with dragging and dropping the files (such as the Drag & Drop overlay not going away or duplicate files, its fix is in PR 2365).

Let me know if you have any notes/questions/ideas

- Removing overlay's border as it is unnecessary and only causing a double border issue on the file input container.
@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

Might be good to have the whole box be clickable instead of just the browse button
and have a on hover event to make the dotted edges bigger/glow etc?

- Remove browse button.
- Make the entire file chooser container clickable.
- Add glowing effect on hover for file chooser.
- Change color of file chooser on hover.
@omar-ahmed42
Copy link
Contributor Author

After making the changes:

Light:

DragAndDropHoverLightcropped

Dark:

DragAndDropHoverDarkcropped

I changed the background color on hover as well as adding a box-shadow (glowing effect) on hover, I tried to make the border thicker on hover, but it didn't look good nor smooth when transitioning from thin border to a thicker one, so I went with the glowing effect instead.

have a on hover event to make the dotted edges bigger/glow etc?

I also removed removed the browse button and made the entire box clickable as mentioned

have the whole box be clickable instead of just the browse button

Let me know if you have any notes/questions/ideas

(The GIF might not be clear due to quality and resolution changes)

@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

Looks good to me! Speedy work too!
/deploypr

Copy link
Contributor

github-actions bot commented Dec 2, 2024

🚀 PR Test Deployment

Your PR has been deployed for testing!

🔗 Test URL: http://185.252.234.121:2368

This deployment will be automatically cleaned up when the PR is closed.

@omar-ahmed42
Copy link
Contributor Author

Now, merge should work successfully, I also fixed the issue where files being removed from merge's list wouldn't reflect in the file chooser.

Copy link
Contributor

github-actions bot commented Dec 2, 2024

🚀 PR Test Deployment

Your PR has been deployed for testing!

🔗 Test URL: http://185.252.234.121:2368

This deployment will be automatically cleaned up when the PR is closed.

@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

Now, merge should work successfully, I also fixed the issue where files being removed from merge's list wouldn't reflect in the file chooser.

files removed from file choose doesn't reflect in merge list though

@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

I think duplicate file viewing on merge doesn't add much value anyway, if we can figure out a way to remove it for merge it would prob be best

Copy link
Contributor

@reecebrowne reecebrowne left a comment

Choose a reason for hiding this comment

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

All working great now as far as I can tell. Thanks for fixing it so promptly!

- Make inputElement optional in removeFileById, this way we could control changing inputElements files.
@omar-ahmed42
Copy link
Contributor Author

Now, merge should work successfully, I also fixed the issue where files being removed from merge's list wouldn't reflect in the file chooser.

files removed from file choose doesn't reflect in merge list though

I believe it's now fixed, it seems like I forgot to do a quick regression testing for file chooser's file removal (as I refactored a little bit of code to reduce duplication when I fixed merge's file removal bug) that's why I didn't notice the bug

@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

/deploypr
Brilliant work though, speedy too! thanks again

Copy link
Contributor

github-actions bot commented Dec 2, 2024

🚀 PR Test Deployment

Your PR has been deployed for testing!

🔗 Test URL: http://185.252.234.121:2368

This deployment will be automatically cleaned up when the PR is closed.

@Frooodle Frooodle enabled auto-merge (squash) December 2, 2024 16:59
@omar-ahmed42
Copy link
Contributor Author

I believe we forgot to include translations for "Click", "or" and "Drag & Drop"

@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

good catch!
if you can add a line to the GB files we have automated script to add to rest of files after merge to dev

@omar-ahmed42
Copy link
Contributor Author

Alright, I will try doing that

@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

Im also happy to do it in another PR after merge if you are busy :P

lemme know!

@omar-ahmed42
Copy link
Contributor Author

omar-ahmed42 commented Dec 2, 2024

It's totally fine, don't worry, I was just stuck with a bug when I was trying to use translations with pseudo element (::after), turns out there was no bug I just placed th:data-text on the wrong HTML element :D

if you can add a line to the GB files we have automated script to add to rest of files after merge to dev
Would adding translation to another file besides GB cause issues?

I already added it to messages_en_GB.properties, but I also added it to messages_ar_AR.properties (I haven't yet committed the changes), if this would cause any issues let me know, if not, let me know as well so that I would push the changes

@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

nope thats all good!

Copy link
Contributor

github-actions bot commented Dec 2, 2024

🚀 Translation Verification Summary

🔄 Reference Branch: pr-branch

📃 File Check: messages_ar_AR.properties

  1. Test Status:Passed
  2. Test Status:Passed

✅ Overall Check Status: Success

Thanks @omar-ahmed42 for your help in keeping the translations up to date.

@omar-ahmed42
Copy link
Contributor Author

You can now merge :D

@Frooodle
Copy link
Member

Frooodle commented Dec 2, 2024

Merge conflict with your other PR i think? #2365

Copy link
Contributor

github-actions bot commented Dec 2, 2024

🚀 Translation Verification Summary

🔄 Reference Branch: pr-branch

📃 File Check: messages_ar_AR.properties

  1. Test Status:Passed
  2. Test Status:Passed

✅ Overall Check Status: Success

Thanks @omar-ahmed42 for your help in keeping the translations up to date.

@omar-ahmed42
Copy link
Contributor Author

Merge conflict with your other PR i think? #2365

Resolved the conflicts, thanks for letting me know

@Frooodle Frooodle merged commit de4637e into Stirling-Tools:main Dec 2, 2024
8 checks passed
@omar-ahmed42
Copy link
Contributor Author

I believe this also closes issue #2372

@reecebrowne reecebrowne linked an issue Dec 2, 2024 that may be closed by this pull request
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Front End Issues or pull requests related to front-end development size:XL This PR changes 500-999 lines, ignoring generated files. Translation
Projects
None yet
3 participants