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(list-item): auto-focus works for <select> elements #101

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

philippfromme
Copy link
Contributor

<select> elements can now be auto-focused.

RMo62gjiSP


Closes #100

@nikku
Copy link
Member

nikku commented Sep 27, 2021

Nica!

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Very nice follow up to what could have been a UX hick-up otherwise 👏.

/CC @andreasgeier

@barmac
Copy link
Member

barmac commented Sep 27, 2021

@philippfromme
Copy link
Contributor Author

Do we need any changes regarding https://github.com/bpmn-io/properties-panel/blob/main/src/components/entries/List.js#L156 ?

Oh, I didn't know about that. So we have the same functionality in two places?

@nikku
Copy link
Member

nikku commented Sep 27, 2021

Oh, I didn't know about that. So we have the same functionality in two places?

If possible, shall move that into a single behavior (or hook).

@barmac
Copy link
Member

barmac commented Sep 27, 2021

Do we need any changes regarding https://github.com/bpmn-io/properties-panel/blob/main/src/components/entries/List.js#L156 ?

Oh, I didn't know about that. So we have the same functionality in two places?

We do, though the behaviour is a bit different.

In a group list, we require to provide an ID of the entry to be focused. So theoretically, this could be a second or third entry of the item. In a nested list, we are not able to tell the ID upfront. Because of that, if you enable the autofocus, it will always be activated on the first input in an item.

@pinussilvestrus
Copy link
Contributor

pinussilvestrus commented Sep 27, 2021

Oh, I didn't know about that. So we have the same functionality in two places?

If possible, shall move that into a single behavior (or hook).

Maybe something we should consider when aligning the behaviors in general (if we want to): #96

/cc @MaxTru

@philippfromme
Copy link
Contributor Author

Okay, I will fix it in both places for now. We'll tackle DRYing up that code in a later refactoring.

@philippfromme
Copy link
Contributor Author

philippfromme commented Sep 27, 2021

Done. @pinussilvestrus or @barmac can you have another look at this?

@barmac
Copy link
Member

barmac commented Sep 27, 2021

Still ✅ from me.

@philippfromme philippfromme merged commit d983c14 into main Sep 27, 2021
@philippfromme philippfromme deleted the 100-auto-focus-select branch September 27, 2021 14:15
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 27, 2021
philippfromme added a commit to bpmn-io/bpmn-properties-panel that referenced this pull request Sep 27, 2021
philippfromme added a commit to bpmn-io/bpmn-properties-panel that referenced this pull request Sep 27, 2021
philippfromme added a commit to bpmn-io/bpmn-properties-panel that referenced this pull request Oct 4, 2021
philippfromme added a commit to bpmn-io/bpmn-properties-panel that referenced this pull request Oct 4, 2021
MaxTru pushed a commit to bpmn-io/bpmn-properties-panel that referenced this pull request Oct 4, 2021
pinussilvestrus pushed a commit to bpmn-io/bpmn-properties-panel that referenced this pull request Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Select Entries Can't Be Auto-Focused
4 participants