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: update picker component roles #6885

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

fix: update picker component roles #6885

wants to merge 8 commits into from

Conversation

JiginJayaprakash
Copy link

@JiginJayaprakash JiginJayaprakash commented Jan 4, 2024 •
edited

Fixes #6884

Pull Request

📖 Description

The roles are updated to listbox and option from list and listitem

🎫 Issues

(#6884)

👩‍💻 Reviewer Notes

📑 Test Plan

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

@JiginJayaprakash
Copy link
Author

@microsoft-github-policy-service agree

@scomea
Copy link
Collaborator

scomea commented Jan 4, 2024

I agree with the menu and the menu items within changing. Still not sure about the list of already selected items (pickerlist and pickerlistItem). Let's discuss in the bug.

@radium-v radium-v changed the title fix for update picker item and picker list roles fix: update picker component roles Jan 5, 2024
KingOfTac
KingOfTac requested changes Jan 8, 2024
View reviewed changes
Copy link
Collaborator

@KingOfTac KingOfTac left a comment

Choose a reason for hiding this comment

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

I agree with the menu and the menu items within changing. Still not sure about the list of already selected items (pickerlist and pickerlistItem). Let's discuss in the bug.

I agree, I think for now we can just change the roles on the menu and menu items. The selected items will need further investigation.

@JiginJayaprakash Can you revert the role changes to pickerlist and pickerlistitem? Once that is done and @radium-v's comments are addressed on the change file I will feel good about approving this change.

JiginJayaprakash
JiginJayaprakash commented Jan 12, 2024
View reviewed changes
Copy link
Author

@JiginJayaprakash JiginJayaprakash left a comment •
edited

Choose a reason for hiding this comment

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

@KingOfTac made suggested changes

@bheston bheston requested a review from KingOfTac March 16, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radium-v radium-v radium-v left review comments

@scomea scomea scomea approved these changes

@bheston bheston bheston approved these changes

@EisenbergEffect EisenbergEffect Awaiting requested review from EisenbergEffect

@chrisdholt chrisdholt Awaiting requested review from chrisdholt chrisdholt is a code owner

@nicholasrice nicholasrice Awaiting requested review from nicholasrice

@KingOfTac KingOfTac Awaiting requested review from KingOfTac KingOfTac is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix: update picker item and picker list roles
5 participants
@JiginJayaprakash @scomea @radium-v @KingOfTac @bheston