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

KDropdown and KIconButton tweaks #465

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

KDropdown and KIconButton tweaks #465

wants to merge 6 commits into from

Conversation

nucleogenesis
Copy link
Member

@nucleogenesis nucleogenesis commented Oct 11, 2023 •
edited
Loading

Description

  • Extend KDropdownButton with a new slot to customize the dropdown options' rendering
  • Drill the containFocus prop through KDropdownButton to UiPopover
  • Add v-bind="$attrs" to KIconButton
  • Add expand/collapse all icons

Issue addressed

This was necessary to accommodate needs for the updates to Kolibri Quiz.

Before/after screenshots

n/a

Steps to test

I suggest testing my Kolibri PR which puts these changes to use.

Testing checklist

  • Contributor has fully tested the PR manually
  • The change has been added to the changelog

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?

nucleogenesis added a commit that referenced this pull request Oct 11, 2023
@nucleogenesis
update changelog to include #465
17f7d82
@nucleogenesis nucleogenesis added category: library Shared code library product: Kolibri Relevant to a specific issue in Kolibri TODO: needs review Waiting for review Component: KDropdownMenu Component: KIconButton labels Oct 11, 2023
nucleogenesis added a commit that referenced this pull request Oct 11, 2023
@nucleogenesis
update changelog to include #465
50a07f3
@nucleogenesis nucleogenesis removed the TODO: needs review Waiting for review label Oct 11, 2023
@nucleogenesis nucleogenesis marked this pull request as draft October 11, 2023 04:54
@nucleogenesis nucleogenesis mentioned this pull request Oct 11, 2023
Merged
9 tasks
MisRob
MisRob reviewed Oct 12, 2023
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
@MisRob
Copy link
Member

MisRob commented Oct 12, 2023

Please note that release-v1.5.x branch was renamed back to main (part of preparation of the new release and returning back to regular release process after that, more details to come)

@MisRob
Copy link
Member

MisRob commented Oct 12, 2023

Alright main is ready to be rebased on

nucleogenesis added a commit that referenced this pull request Oct 12, 2023
@nucleogenesis
update changelog to include #465
83c9f97
akolson
akolson reviewed Oct 16, 2023
View reviewed changes
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Additions look correct to me. Thanks @nucleogenesis!

nucleogenesis added a commit that referenced this pull request Oct 26, 2023
@nucleogenesis
update changelog to include #465
9e14947
@nucleogenesis nucleogenesis marked this pull request as ready for review October 26, 2023 05:20
@marcellamaki
Copy link
Member

@nucleogenesis although I can follow the code changes, I'm a little bit unclear on what exactly you added in the create quiz PR that required this change? Can you explain a little bit? I have it running and also am looking at the code but just wanted to be sure I understand the use case. (Relatedly, adding a sentence or two to the documentation to the same effect would be helpful)

@nucleogenesis
v-bind attrs on KIconButton
f44d921
@nucleogenesis
KDropdownMenu expose a slot that provides the option as a slot-prop
5c05d55 
This allows us to pass whatever we want in as the children of the KDropdownMenu
@nucleogenesis
KDropdownMenu drill containFocus prop into UiPopover
abb8af1 
Rather than defaulting to true, give the user the option here
nucleogenesis added a commit that referenced this pull request Nov 16, 2023
@nucleogenesis
update changelog to include #465
a85216f
@nucleogenesis
Copy link
Member Author

Hey @marcellamaki I've updated the changelog & docs page to better explain the changes to KDropdownMenu and to offer guidance re: the changes -- thank you for the feedback there I hadn't considered how this would be good to note in the docs 🤦🏻‍♂️

@MisRob
Copy link
Member

MisRob commented Nov 17, 2023

I'm sorry for being a pain in the butt, but the changelog still needs cleanup, @nucleogenesis. There's still a conflict caused by introducing the new changelog format while this branch was probably in progress already. Could you please remove lines 272-291, and line 294 completely? That's the old format that has already been converted to new items. Let me know if you needed anything.

Your guidance in the new changelog item for this PR looks great, thank you.

MisRob
MisRob reviewed Nov 17, 2023
View reviewed changes
CHANGELOG.md
@@ -7,6 +7,17 @@ Changelog is rather internal in nature. See release notes for the public overvie

## Version 2.0.0

- [#465]
- **Description:** KDropdownMenu improvements & bind $attrs in KIconButton
- **Products impact:** -
Copy link
Member

Choose a reason for hiding this comment

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

I think this change would fall into "updated API" products impact category since there are some updates to public facing API for components like a new slot and attributes binding

MisRob
MisRob reviewed Nov 17, 2023
View reviewed changes
CHANGELOG.md Outdated
- **Components:** KDropdownMenu, KIconButton
- **Breaking:** no
- **Impacts a11y:** yes
- **Guidance:** The KIconButton simply allows you to ensure that attrs you bind to it will be applied to the resulting button itself (ie, tabindex). KDropdownMenu now emits the @tab event when the [Tab] key is pressed and @close when the popover is programmatically closed. This allows more precise focus management as the popover by default could end up sending focus to the root HTML element. Furthermore, KDropdownMenu now exposes a #option slot. This slot exposes the current option so a template snippet can be passed here to customize the display of items in the menu. This could also be used to show a list of checkboxed items.
Copy link
Member

Choose a reason for hiding this comment

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

👍 🙏

@nucleogenesis
Copy link
Member Author

Just a note that I will be splitting this PR into two separate PRs -- one which is necessary specifically for Kolibri work and the other which represents unused changes that were inspired during Kolibri work but went unused. I'll update both changelogs accordingly next week and request re-review from the participants here.

@nucleogenesis nucleogenesis mentioned this pull request Nov 28, 2023
Merged
1 task
@nucleogenesis nucleogenesis added the type: proposal New feature or request label Nov 28, 2023
@nucleogenesis
Copy link
Member Author

@marcellamaki @MisRob I've extracted the commits from this PR for the quiz management work into #497 so that these changes appear separately from the others in the changelog.

@MisRob
Copy link
Member

MisRob commented Dec 7, 2023

@nucleogenesis I am not sure if I could understand the motivation for two pull requests properly. Do you need anything in this one?

@MisRob
Copy link
Member

MisRob commented Feb 2, 2024

If it's still active and has no breaking updates, please retarget to release-v3 branch before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akolson akolson akolson left review comments

@MisRob MisRob MisRob left review comments

@marcellamaki marcellamaki Awaiting requested review from marcellamaki

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
category: library Shared code library Component: KDropdownMenu Component: KIconButton product: Kolibri Relevant to a specific issue in Kolibri type: proposal New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nucleogenesis @MisRob @marcellamaki @akolson