-
Notifications
You must be signed in to change notification settings - Fork 86
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
Update kdropdown menu api #346
Update kdropdown menu api #346
Conversation
@marcellamaki Thank you. Marcella, could you please make sure to fix the linter check? There are some prettier issues with spacing that make previewing changes difficult so it'd be helpful for review. |
Yes - I will do this today. Thanks for pointing this out @MisRob, I'd missed it, and it definitely makes the diff seem much larger than it is! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @marcellamaki, we reviewed the code together with @akolson, and overall it's looking good. There were a couple of places where we asked some questions, and it's possible that no updates will be required there, only clarification.
Also, one more general question regarding the PR description, what did you mean by "Fix the regression where the additional options for the button (previously in a in KDropdownMenu is no longer working, as you can see in the documentation"?
Before merging though, we'll need to make sure that the changelog and documentation are updated to reflect API updates for all KButton
, KIconButton
, and KDropdownMenu
. This may not be a full list, just a pointer to a couple of places I noticed:
1. Nothing shows up in the Dropdowns section of the Buttons and links page and the section text is obsolete (https://deploy-preview-346--kolibri-design-system.netlify.app/buttons#dropdowns)
2. The Overview section on the KDropdownMenu
page is obsolete (https://deploy-preview-346--kolibri-design-system.netlify.app/kdropdownmenu)
3. menu
slot needs to be documented on KButton
and KIconButton
pages (https://deploy-preview-346--kolibri-design-system.netlify.app/kiconbutton/#slots)
4. I'd suggest mentioning how KButton
, KIconButton
, and KDropdownMenu
can be used together including a code example, ideally. This could be, for example, done on the KDropdownMenu
page and linked to from KButton
and KIconButton
pages. Or in any other way. The goal of this feedback is to make sure that everyone can understand easily how to use these components together.
This also fixes #67 |
@MisRob and @akolson - what I am referencing (not very clearly) is this part of the documentation on "Buttons" where you can see: |
@marcellamaki I see, okay, thank you |
…n, the tooltip can be repositioned for no overlap
d31bf6a
to
2cd2da0
Compare
- adds missing documentation to 'KIconButton' - fixes 'menu' slot documentation not being generated for 'KButton' - uses language consistent with other slots documentation
As @marcellamaki is on vacation, instead of posting feedback on the newest changes, I rebased on top of the latest |
`disabled` prop has been deprecated in learningequality/kolibri-design-system#346
Follow-up to learningequality#346 Fixes 'Property or method "disabled" is not defined on the instance but referenced during render.'
Follow-up to learningequality#346 where we removed KButton from KDropdownMenu. As a result, products now need to wrap KDropdownMenu in KButton and therefore need to use 'hasDropdown' property for dropdown menus that are within buttons. This commit exposes this prop in documentation.
and clean up documentation (keep information about recent changes in the changelog rather than in docs + couple minor updates) Follow-up to learningequality#346
default slot is not alternative to `text` anymore Follow-up to learningequality#346
Follow-up to learningequality#346 Fixes 'Property or method "disabled" is not defined on the instance but referenced during render.'
Follow-up to learningequality#346 where we removed KButton from KDropdownMenu. As a result, products now need to wrap KDropdownMenu in KButton and therefore need to use 'hasDropdown' property for dropdown menus that are within buttons. This commit exposes this prop in documentation.
"Update kdropdown menu api #346" follow-up
Description
This PR changes the KDropdownMenu API by addressing the issue that KDropdownMenu also contained a button. Now, KDropdownMenu is only the popover elements
Issue addressed
Addresses #164
Does a better job fixing #136 (which caused a regression with my last fix).
Fix the regression where the additional options for the button (previously in a
<slot/>
in KDropdownMenu is no longer working, as you can see in the documentationBefore/after screenshots
There are no visual changes to the UI (I hope!)
Steps to test
I will open a corresponding kolibri PR that points to this commit next week with instructions for testing.
(optional) Implementation notes
At a high level, how did you implement this?
The makes
KDropdownMenu
a slot forKButton
(orKIconButton
) rather than responsible for also creating the button.Does this introduce any tech-debt items?
Doing this would require some (fairly straightforward) remediation following a KDS version bump in Kolibri (11 instances of component are used) and KDP (5 instances of component). None in Studio.
Testing checklist
changelog
Reviewer guidance
Post-merge updates and tracking
Comments