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(dropdown): Various dropdown fixes #434

Merged
merged 8 commits into from
Jul 19, 2022
Merged

Conversation

rkaraivanov
Copy link
Member

  • Dropdown now supports navigation with Home/End keys.
  • Dropdown now supports initial selection through selected attribute
    on its items. In case of multiple items, the last in the DOM hierarchy
    wins.
  • Fixed an issue with igcChange being emitted twice when making selection.
  • Fixed dropdown opened/closed events to be emitted after the underlying
    DOM is updated.
  • Fixed dropdown closing events not being emitted when clicking around
    the page with keepOpenOnOutsideClick set to false.
  • Fixed toggle directive sameWidth option to correctly reset the floater
    element styles after being turned off.

Closes #413
Closes #414
Closes #419

* Dropdown now supports navigation with Home/End keys.
* Dropdown now supports initial selection through `selected` attribute
  on its items. In case of multiple items, the last in the DOM hierarchy
  wins.
* Fixed an issue with `igcChange` being emitted twice when making selection.
* Fixed dropdown `opened/closed` events to be emitted after the underlying
  DOM is updated.
* Fixed dropdown closing events not being emitted when clicking around
  the page with `keepOpenOnOutsideClick` set to false.
* Fixed toggle directive `sameWidth` option to correctly reset the floater
  element styles after being turned off.
simeonoff
simeonoff previously approved these changes Jul 14, 2022
Copy link
Member

@damyanpetev damyanpetev left a comment

Choose a reason for hiding this comment

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

PS: technically a feature + various fixes in the PR, if you want to align commits for the changelog.

@@ -26,7 +26,6 @@ export default class IgcDropdownGroupComponent extends LitElement {
@queryAssignedElements({ flatten: true, selector: 'igc-dropdown-item' })
public items!: Array<IgcDropdownItemComponent>;

/** @private */
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this interferes with anything, but if possible do leave it private. The groups and items under a dropdown are supposed to strictly inherit the size of the parent and doesn't make sense for them to be different. Guessing this is exposed to facilitate that and if or should I say when we tackle the global size through themes this will inherit from cascade and be obsoleted/removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe as an alternative make the group items get the size from their parent instead of exposing an additional attribute.
Even with the private tag this would show in auto-complete in editors. Not to mention that the dropdown overwrites the value anyway...

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, the way you refactored it is likely better, however.. when the child looks for its parent explicitly it won't be super friendly for extended components (looking at the select, combo maybe). Perhaps generalize the parent with some base interface and move the query for it in a protected getParent() or something similar.

src/components/toggle/toggle.controller.ts Outdated Show resolved Hide resolved
src/components/toggle/toggle.controller.ts Outdated Show resolved Hide resolved
src/components/dropdown/dropdown.ts Outdated Show resolved Hide resolved
src/components/dropdown/dropdown.ts Show resolved Hide resolved
src/components/dropdown/dropdown.ts Show resolved Hide resolved
@rkaraivanov rkaraivanov marked this pull request as ready for review July 15, 2022 07:39
@damyanpetev damyanpetev merged commit 8819d11 into master Jul 19, 2022
@damyanpetev damyanpetev deleted the rkaraivanov/dropdown-fixes branch July 19, 2022 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants