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

feat(files): rename files and directories #2445

Merged
merged 11 commits into from
Mar 25, 2022
Merged

feat(files): rename files and directories #2445

merged 11 commits into from
Mar 25, 2022

Conversation

josephmcg
Copy link
Contributor

@josephmcg josephmcg commented Mar 22, 2022

🔴 You will need to add new files after switching to this branch. You won't be able to fetch the file on full screen view for old items

What this PR does 📖

  • new modal to rename items on right click. Works in both grid and list view
    • throw errors as expected if name already exists, etc
    • file system counts a.txt and a.TXT as different files. I made a new ticket for case sensitive duplicate name checks
  • clean up old dummy rename emits/methods
  • encourage user to keep extension in name by highlighting specific text, but they can remove if desired
    • on download, it will add the correct extension if the user removed/changed it
  • removed hash property from Fil class. We were using it previously for unencrypted files.
  • set file path in bucket based on item id (uuid). was based on name before which caused renaming issues
  • Change renameChild to use a more typical guard clause
  • update mobile half size modal css. Will only take up necessary room now, much better looking. Can be seen in glyph details (click a glyph in chat) and rename modal

Which issue(s) this PR fixes 🔨
AP-797

Special notes for reviewers 🗒️

  • if archive file extension is removed from name, it will only show regular icon for now. I made a new ticket for this
  • Items are technically removed and added as a child, changing the order of items. This will be fixed later, with the Recent filter on the right

Additional comments 🎤

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Mar 22, 2022
@netlify
Copy link

netlify bot commented Mar 22, 2022

Yeeeehaw, deploy preview is ready!

Name Link
🔨 Latest commit e03a44f
🔍 Latest deploy log https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/623bc905a03e9f00086e6ff5
😎 Deploy Preview https://deploy-preview-2445--adoring-edison-dbcef8.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@josephmcg

This comment was marked as outdated.

@stavares843
Copy link
Member

/rebase

@molimauro
Copy link
Contributor

Great job Joe. I found a bug when you open the preview of a file i cannot close it anymore, the buttons on top right are gone! Other than that we're good to go

@molimauro molimauro added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Mar 23, 2022
@josephmcg
Copy link
Contributor Author

josephmcg commented Mar 23, 2022

Thanks Mauro! I think that bug was introduced by my PR the other day. Phil opened a ticket for it (AP1150)

There's a chance it's due to some other update since it's happening for the fullscreen file view in chat as well, I'm not sure

@molimauro
Copy link
Contributor

Oh great then! Yeah could be related to the swiper library update, we had also other problems related to that in the chat/file view.

@molimauro molimauro added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Mar 23, 2022
@phillsatellite
Copy link
Contributor

phillsatellite commented Mar 23, 2022

Tested: Works good but found a couple issues,

  1. I can save a rename a file to no name (or just spaces) and it will still create. If a file name is just ".png" and I try to rename it to that same name it will say "Already a file with this name"
saving.with.spaces.mov
  1. On an actual iOS device when I click rename the screen will blur but no rename modal will appear
    (you will have to download this video Github doesnt like my screenrecording on mobile devices)
RPReplay_Final1648057301.mov

@phillsatellite phillsatellite added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. labels Mar 23, 2022
@josephmcg
Copy link
Contributor Author

@phillsatellite The name issue should be resolved. It was initially setup to trim white space on names. I changed it to preserve whitespace (similar to how mac os handles it). However, mac will prevent you from naming something .png. I could try to address this separately if we don't like it.

Unsure of the modal issue, I added something that hopefully fixes it. waiting for netlify build to finish

@josephmcg
Copy link
Contributor Author

Yep, css fix worked on my iphone

@josephmcg josephmcg added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Mar 24, 2022
@phillsatellite
Copy link
Contributor

Hi @josephmcg ! I tested again and iOS issue was fixed! But one little issue with it, the modal will appear at the bottom of the screen, I asked Liz and she had said "Yes it should appear in the middle. But not behind the keyboard. So like if the keyboard disrupts it or happens to sit on top of it, it should push it up" Lemme know if that clarified it enough if not feel free to DM me 😄
ios

@phillsatellite phillsatellite added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. labels Mar 24, 2022
@josephmcg josephmcg added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Mar 25, 2022
@josephmcg
Copy link
Contributor Author

the keyboard will more or less push it up to the middle. happy to make any additional edits if we receive clarification

@stavares843
Copy link
Member

Captura de ecrã 2022-03-25, às 20 02 20

i know this is already on https://github.com/Satellite-im/Core-PWA/blob/dev/libraries/Files/errors/Errors.ts#L9 but maybe we could edit for something more friendly

@stavares843
Copy link
Member

Captura de ecrã 2022-03-25, às 20 03 10

when user reaches the max char number doesn't return any feedback to the user

@stavares843
Copy link
Member

i can add a folder with a * but I can't rename with a *
Captura de ecrã 2022-03-25, às 20 18 43

Captura de ecrã 2022-03-25, às 20 18 37

@stavares843
Copy link
Member

stavares843 commented Mar 25, 2022

rename is pretty neat 🎉

Captura de ecrã 2022-03-25, às 20 30 25

im merging this, we can add those minor improvements I mentioned above separately

@stavares843 stavares843 merged commit 2dce9b5 into dev Mar 25, 2022
@stavares843 stavares843 deleted the AP797 branch March 25, 2022 20:31
@github-actions github-actions bot removed the Ready for QA Ready for QA team to test, Devs approved. label Mar 25, 2022
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.

None yet

4 participants