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

File rename refactor #2054

Merged
merged 12 commits into from
Apr 1, 2022
Merged

File rename refactor #2054

merged 12 commits into from
Apr 1, 2022

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Mar 29, 2022

closes #2050

Since the edition isn't exactly instant even with good internet, I find it good to have a spinner to show something is happening


Submission checklist:

Layout

  • Change looks good in the desktop web ui
  • Change looks good in the mobile web ui

Theme

  • Components / elements inspected in light mode
  • Components / elements inspected in dark mode

@render
Copy link

render bot commented Mar 29, 2022

@render
Copy link

render bot commented Mar 29, 2022

@render
Copy link

render bot commented Mar 29, 2022

@github-actions github-actions bot added the Type: Bug Fix Added to PRs if they are addressing a bug label Mar 29, 2022
@asnaith
Copy link
Member

asnaith commented Mar 30, 2022

I think this feels more natural 👍.

There is no way for a user to cancel once they have made their change now though.

We also need to apply the same change to the Shared page, when renaming a Shared Folder it did not save when I clicked outside the input.

It would be good to apply the same change to files within buckets on Storage UI too (or create a seperate issue), what do you think?

I wasn't able to see the spinner when I throttled the connection in the dev tools.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Mar 30, 2022

There is no way for a user to cancel once they have made their change now though.

There is by pressing the esc key, like on an os.

Thanks for checking it out, I'll apply these changes to the other areas where we rename stuff.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Mar 30, 2022

Storage needed some small refactor. This is being a big PR now. I tried to test all cases, and couldn't spot anything missing, feel free to play around and rename anything you can rename :)

I did not change the mobile way with a loader on the modal, just to keep things from exploding totally.

@asnaith
Copy link
Member

asnaith commented Mar 30, 2022

Just checked out the latest changes. Share folder renaming in Files UI and file renaming in Storage UI buckets are both good now.

I did notice on storage ui that we're not doing the same thing with file extensions when we're renaming so I'll create a separate issue as it would be good to have them behaving the same.

I realized I was seeing the spinner before but in dark mode it's really difficult to see:

Screen Shot 2022-03-30 at 2 55 00 PM

@tanmoyAtb
Copy link
Contributor

tanmoyAtb commented Mar 31, 2022

The implementation is really great, here are some thoughts that came into my mind.
These are really borderline nitpicking though.

  1. on hitting rename, the editing field should get in focus (I start typing right after hitting rename).
  2. If you clear the field, and click outside, this should cancel the rename operation (This is common OS behaviour as well, sth I noticed when I didn't want to hit esc).

…ChainSafe/ui-monorepo into fix/tbaut-save-on-blur-edit-loader-2050
@Tbaut
Copy link
Collaborator Author

Tbaut commented Mar 31, 2022

@tanmoyAtb

the editing field should get in focus

Agreed, I looked for this, the autofocus is set properly, but there's generally a focus problem linked to the drag and drop IMHO, so formik fails at focusing the field. We should double check this with #2051

If you clear the field, and click outside, this should cancel the rename operation (This is common OS behaviour as well, sth I noticed when I didn't want to hit esc).

Ah good point! Definitely something I can do

@asnaith I hadn't tested dark mode yet (and didn't check the box in the first post :D ), it's fixed now :)
image

@asnaith
Copy link
Member

asnaith commented Mar 31, 2022

If you clear the field, and click outside, this should cancel the rename operation (This is common OS behaviour as well, sth I noticed when I didn't want to hit esc).

Ah good point! Definitely something I can do

This seems implemented already :)

canren.mov

@asnaith I hadn't tested dark mode yet (and didn't check the box in the first post :D ), it's fixed now :)

That's why I switched to dark mode straight away 😂. Looks great now.

const newName = extension !== "" ? `${values.name.trim()}.${extension}` : values.name.trim()

if (newName !== name) {
newName && handleRename && handleRename(file.cid, newName)
if (newName !== name && !!newName && handleRename) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup that's in the code, if newName is "" it'll be falsy and go to the else and stop editing.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Mar 31, 2022

Thanks Andrew for checking, it's indeed already supported :)

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

The rename functionality feels fully intuitive now. 💯

…leSystemItem/FileSystemGridItem.tsx

Co-authored-by: Tanmoy Basak Anjan <[email protected]>
@Tbaut Tbaut merged commit d0975ed into dev Apr 1, 2022
@Tbaut Tbaut deleted the fix/tbaut-save-on-blur-edit-loader-2050 branch April 1, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Fix Added to PRs if they are addressing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Click outside should save a renaming action
4 participants