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.

Sign up for GitHub

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

Jump to bottom

Fix #6124 : Dropdown navigation like PrimeVue #6430

Merged
merged 9 commits into from
Apr 29, 2024
Merged

Fix #6124 : Dropdown navigation like PrimeVue #6430

merged 9 commits into from
Apr 29, 2024

Conversation

KirilCycle
Copy link
Contributor

@KirilCycle KirilCycle commented Apr 20, 2024 •
edited by melloware
Loading

Fix #6124
Fix #6417
Each Group item now have unique index

@vercel Vercel
Copy link

vercel bot commented Apr 20, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
primereact ⬜️ Ignored (Inspect) Visit Preview Apr 23, 2024 0:42am
primereact-v9 ⬜️ Ignored (Inspect) Visit Preview Apr 23, 2024 0:42am

@KirilCycle KirilCycle changed the title Fix: Dropdown navigation Fix #6124 : Dropdown navigation Apr 20, 2024
@KirilCycle
Copy link
Contributor Author

@melloware Hello! Its done

melloware
melloware requested changes Apr 21, 2024
View reviewed changes
components/lib/dropdown/Dropdown.js Outdated Show resolved Hide resolved
components/lib/dropdown/Dropdown.js Outdated Show resolved Hide resolved
components/lib/dropdown/DropdownPanel.js Outdated Show resolved Hide resolved
@melloware
Copy link
Member

@KirilCycle i think this may also fix this issue correct? #6417

@KirilCycle
Copy link
Contributor Author

@KirilCycle i think this may also fix this issue correct? #6417

Exactly!

@melloware melloware added the Type: Bug Issue contains a defect related to a specific component. label Apr 21, 2024
@melloware melloware mentioned this pull request Apr 21, 2024
Closed
@melloware
Copy link
Member

Let me take a deeper look at this. I like where you are going though.

@KirilCycle
Copy link
Contributor Author

KirilCycle commented Apr 21, 2024 •
edited
Loading

Let me take a deeper look at this. I like where you are going though.

Here also second approach, we can use simple number for key at simple list, but for group list we can use key like ${groupIndex}_${index}

@melloware
Copy link
Member

I think we need to study PrimeVue they actually do this: https://github.com/primefaces/primevue/blob/344ec64cd461aad1493073dd3ff10bc8a7338654/components/lib/dropdown/Dropdown.vue#L879-L888

And the Groups are actually a different type of Option that has result.push({ optionGroup: option, group: true, index }); so they index everything properly in getVisibleOptions

https://github.com/primefaces/primevue/blob/344ec64cd461aad1493073dd3ff10bc8a7338654/components/lib/dropdown/Dropdown.vue#L902-L926

@KirilCycle
Copy link
Contributor Author

KirilCycle commented Apr 21, 2024 •
edited
Loading

I think we need to study PrimeVue they actually do this: https://github.com/primefaces/primevue/blob/344ec64cd461aad1493073dd3ff10bc8a7338654/components/lib/dropdown/Dropdown.vue#L879-L888

And the Groups are actually a different type of Option that has result.push({ optionGroup: option, group: true, index }); so they index everything properly in getVisibleOptions

https://github.com/primefaces/primevue/blob/344ec64cd461aad1493073dd3ff10bc8a7338654/components/lib/dropdown/Dropdown.vue#L902-L926

Should I try to apply it the way it's done in vue example?

@melloware
Copy link
Member

Yes we should see if we can modify things the Vue way. If you try the Vue showcase the Dropdown with Groups is working fine: https://primevue.org/dropdown/#group

@KirilCycle
Copy link
Contributor Author

Yes we should see if we can modify things the Vue way. If you try the Vue showcase the Dropdown with Groups is working fine: https://primevue.org/dropdown/#group

Okey ill take a look

@KirilCycle
Copy link
Contributor Author

KirilCycle commented Apr 21, 2024 •
edited
Loading

@melloware Vue Dropdown will flat all this to single options list first, and then all logic will work with this flatted list, But the React Dropdown component use the same format as it was passed and functions are not able to work with this type of data, also DropdownPanel is designed to handle groups the way they are, however still has an issue with indexses.I guess we can achieve the similar behaviour to Vue. Let me keep working

@melloware
Copy link
Member

Yes I saw that. I feel like Vue is doing it a cleaner way and we just need to tweak the React code to do the same thing. I think DropDownPanel will need some changes but I think in the long run this will be better.

KirilCycle
KirilCycle commented Apr 22, 2024
View reviewed changes
components/lib/dropdown/Dropdown.js Outdated
if (hasFilter && !isLazy) {
const filterValue = filterState.trim().toLocaleLowerCase(props.filterLocale);
const searchFields = props.filterBy ? props.filterBy.split(',') : [props.optionLabel || 'label'];

if (props.optionGroupLabel) {
let filteredGroups = [];

for (let optgroup of props.options) {
for (let optgroup of options) {
Copy link
Contributor Author

@KirilCycle KirilCycle Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Did not check with filter functionality yet

components/lib/dropdown/DropdownPanel.js Outdated Show resolved Hide resolved
melloware
melloware requested changes Apr 22, 2024
View reviewed changes
components/lib/dropdown/Dropdown.js Outdated Show resolved Hide resolved
components/lib/dropdown/DropdownPanel.js Outdated Show resolved Hide resolved
components/lib/dropdown/Dropdown.js Outdated Show resolved Hide resolved
@melloware
Copy link
Member

Wow this is looking so much cleaner!

@melloware
Copy link
Member

Oh run npm run format on the code.

@melloware melloware changed the title Fix #6124 : Dropdown navigation Fix #6124 : Dropdown navigation like PrimeVue Apr 23, 2024
@nitrogenous nitrogenous self-requested a review April 25, 2024 14:15
@nitrogenous nitrogenous added the Status: Pending Test Issue or pull request is being tested by Core Team label Apr 25, 2024
nitrogenous
nitrogenous approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@nitrogenous nitrogenous left a comment

Choose a reason for hiding this comment

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

LGTM. Amazing job thx!

@nitrogenous nitrogenous merged commit b5ed11d into primefaces:master Apr 29, 2024
6 checks passed
@nitrogenous nitrogenous added this to the 10.6.4 milestone Apr 29, 2024
@nitrogenous nitrogenous removed the Status: Pending Test Issue or pull request is being tested by Core Team label Apr 29, 2024
@nitrogenous nitrogenous removed this from the 10.6.4 milestone Apr 30, 2024
@Rekl0w Rekl0w mentioned this pull request May 5, 2024
Closed
@leoo1992 leoo1992 mentioned this pull request May 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melloware melloware melloware approved these changes

@nitrogenous nitrogenous nitrogenous approved these changes

Assignees
No one assigned
Labels
Type: Bug Issue contains a defect related to a specific component.
Projects
None yet
Milestone
No milestone
3 participants
@KirilCycle @melloware @nitrogenous