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

feat(picker-column-option): add the new component #28591

Merged
merged 9 commits into from
Nov 30, 2023
Merged

feat(picker-column-option): add the new component #28591

merged 9 commits into from
Nov 30, 2023

Conversation

thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Nov 28, 2023

Issue number: internal


What is the current behavior?

Users have to pass the options key in an object to generate the buttons within picker-column-internal.

What is the new behavior?

  • Created a stubbed ion-picker-column-option
  • Added an Axe test

The new component will replace the need to pass options as a key. Since the component is stubbed, it won't have functionality and styles. Those will be applied at a later time.

Does this introduce a breaking change?

  • Yes
  • No

The changes are being pushed to a branch that targets a major.

Other information

N/A

@github-actions github-actions bot added the package: core @ionic/core package label Nov 28, 2023
@github-actions github-actions bot added package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Nov 28, 2023
@thetaPC thetaPC marked this pull request as ready for review November 29, 2023 00:59
@amandaejohnston
Copy link
Contributor

Made a dev build: 7.5.7-dev.11701270755.14f050ad

Copy link
Contributor

@amandaejohnston amandaejohnston left a comment

Choose a reason for hiding this comment

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

Tested in core and all three frameworks, looks good 👍

@Prop() value?: any | null;

componentWillLoad() {
this.inheritedAttributes = inheritAriaAttributes(this.el);
Copy link
Contributor

Choose a reason for hiding this comment

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

As per the ticket, we should be taking advantage of Stencil's watch decorator for attributes: https://stenciljs.com/docs/reactive-data#watching-native-html-attributes

Copy link
Contributor

Choose a reason for hiding this comment

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

To add to this, you will still need to get the initial state of the attribute you are trying to reflect. inheritAttributes would be the better utility for this here, since you only are reflecting the aria-label (where as inheritAriaAttributes will parse all aria attributes on the element node). I would assign this to a @State variable, so a mutation to this variable in the @Watch function will trigger a re-render to reflect the aria label to the inner button.

The desired behavior is if the developer changes the aria-label after the initial render of the component, that it will still update in the shadow dom template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you both for the deeper explanations. I now have a better idea on how this stuff works.

Copy link
Contributor

@amandaejohnston amandaejohnston left a comment

Choose a reason for hiding this comment

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

LGTM aside from Liam and Sean's comments.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

As I get into the integration of this component I may make some adjustments to how we set the initial value of ariaLabel, but I think the stub for this looks good!

A couple lingering things, but otherwise good to go once addressed. Great work!

@State() ariaLabel?: string | null = null;

/**
* If `true`, the user cannot interact with the select option.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If `true`, the user cannot interact with the select option.
* If `true`, the user cannot interact with the picker column option.

const { value, disabled, ariaLabel } = this;

return (
<Host id={this.optionId} class={getIonMode(this)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be used anywhere. Can we remove it? (Or document why it's needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added them for future usage. I'll remove them since this is a stubbed version.

Copy link
Contributor

Choose a reason for hiding this comment

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

If they are for future usage I'm fine keeping them, I was just hoping we could document why it was needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that:

  • id would be necessary for children components that act like an option in order to submit the right value
  • getIonMode() would be necessary for styling based on mode

But since I'm not 100% sure if my reasoning is correct and that it's currently not being used, then I removed it.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Thank you!

@thetaPC thetaPC merged commit 0b46964 into FW-5575 Nov 30, 2023
44 checks passed
@thetaPC thetaPC deleted the FW-5579 branch November 30, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: react @ionic/react package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants