Skip to content

[base-ui][Autocomplete] Implement undo / redo#35636

Closed
hbjORbj wants to merge 5 commits intomui:masterfrom
hbjORbj:enh/autocomplete-undo-redo
Closed

[base-ui][Autocomplete] Implement undo / redo#35636
hbjORbj wants to merge 5 commits intomui:masterfrom
hbjORbj:enh/autocomplete-undo-redo

Conversation

@hbjORbj
Copy link
Contributor

@hbjORbj hbjORbj commented Dec 26, 2022

Follow-up on #35545 (comment)

This PR contains:

  • a new hook: useUndoAndRedo
  • using useUndoAndRedo in useAutocomplete

Before:

After:

@hbjORbj hbjORbj added scope: autocomplete Changes related to the autocomplete. This includes ComboBox. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Dec 26, 2022
@mui-bot
Copy link

mui-bot commented Dec 26, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-35636--material-ui.netlify.app/

@material-ui/core: parsed: +0.15% , gzip: +0.17%
@material-ui/lab: parsed: +0.30% , gzip: +0.30%
@material-ui/unstyled: parsed: +0.67% , gzip: +0.79%
@material-ui/utils: parsed: +6.35% , gzip: +5.31%

Details of bundle changes

Generated by 🚫 dangerJS against fd07d76

@hbjORbj hbjORbj self-assigned this Dec 26, 2022
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

The implementation looks clean and promising, however, there needs to be some polish. I ended up in this state:

image

Steps:

  1. Choose Godfather: Part II
  2. Remove it
  3. Choose Pulp Fiction
  4. Press CTRL + Z

import isEqual from 'lodash/isEqual';
import { useState } from 'react';

const useUndoAndRedo = <T>(initialValue?: T) => {
Copy link
Member

Choose a reason for hiding this comment

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

We may want to add an option about how many steps in the history we want to keep.

@hbjORbj
Copy link
Contributor Author

hbjORbj commented Dec 27, 2022

Steps:
Choose Godfather: Part II
Remove it
Choose Pulp Fiction
Press CTRL + Z

Did you remove it by deleting text or by clicking "X" button? In my case, if I remove via clicking "X" button and then undo, all works as expected, but if I remove via deleting text a bug and then undo, I experience the same bug as you did.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I couldn't test the behavior because I have an azerty keyboard. I have left a comment on how to fix this.

Otherwise, I struggle to find end-user traction for this behavior. For example, JedWatson/react-select#168 has little activity, I couldn't find something on select2, bootstrap, and I couldn't find anything else when I search on Google. So I'm not entirely sure that we should build this feature, maybe this has a too high opportunity cost compared to our options?

@hbjORbj
Copy link
Contributor Author

hbjORbj commented Dec 27, 2022

So I'm not entirely sure that we should build this feature @oliviertassinari

But Gmail has it :) Should I open an issue and wait for upvotes?

@michaldudak
Copy link
Member

It's a nice quality-of-life feature! I agree that perhaps there isn't a big market for it, though.
UX-wise, I found one behavior that I didn't expect:

  1. Select The Godfather
  2. Select Pulp Fiction
  3. Undo
  4. Select 12 Angry Men
  5. Undo
  6. Pulp Fiction is selected, despite being undone in point 3.

Whenever you are in the middle of the undo stack and make an edit, you should disregard any items above the current item in the stack.

There's also some weird behavior in the free solo mode - when I select items from the list and enter my own ones, the ones outside the list are not taken into consideration for the undo stack.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 1, 2023

But Gmail has it :) Should I open an issue and wait for upvotes?

It's simply that it's not all free, e.g. it has a bundle size cost:

Screenshot 2023-01-01 at 20 27 54

https://mui-dashboard.netlify.app/size-comparison?circleCIBuildNumber=467289&baseRef=master&baseCommit=622cc969cca7b05315887041f5c9298bbd70cac0&prNumber=35636

If we don't move forward, I think that such issue would be a great next step.

Something I wonder about is: How does the undo shortcut fits when the app also has such logic, e.g. Toolpad? There needs to be a way to not handle the event twice. I guess it can be solved with either an event.stopPropagation or an event.defaultMuiPrevented.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 31, 2023
@oliviertassinari oliviertassinari added design This is about UI or UX design, please involve a designer. and removed design: ux labels Aug 18, 2023
@danilo-leal danilo-leal changed the title [Autocomplete] Implement undo / redo [base-ui][Autocomplete] Implement undo / redo Dec 28, 2023
@danilo-leal danilo-leal added the package: @mui/base Specific to @mui/base (legacy). label Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

design This is about UI or UX design, please involve a designer. package: @mui/base Specific to @mui/base (legacy). PR: out-of-date The pull request has merge conflicts and can't be merged. scope: autocomplete Changes related to the autocomplete. This includes ComboBox. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants