-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
Changes from 6 commits
0ff54b2
9e53c07
fa5076b
decd0f4
0a2c2b0
4f050ad
6777923
841ea51
04a0760
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
import type { ComponentInterface } from '@stencil/core'; | ||
import { Component, Element, Host, Prop, h } from '@stencil/core'; | ||
import type { Attributes } from '@utils/helpers'; | ||
import { inheritAriaAttributes } from '@utils/helpers'; | ||
|
||
import { getIonMode } from '../../global/ionic-global'; | ||
|
||
@Component({ | ||
tag: 'ion-picker-column-option', | ||
shadow: true, | ||
}) | ||
export class PickerColumnOption implements ComponentInterface { | ||
private optionId = `ion-picker-opt-${pickerOptionIds++}`; | ||
|
||
private inheritedAttributes: Attributes = {}; | ||
|
||
@Element() el!: HTMLElement; | ||
|
||
/** | ||
* If `true`, the user cannot interact with the select option. | ||
*/ | ||
@Prop() disabled = false; | ||
|
||
/** | ||
* The text value of the option. | ||
*/ | ||
@Prop() value?: any | null; | ||
|
||
componentWillLoad() { | ||
this.inheritedAttributes = inheritAriaAttributes(this.el); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. The desired behavior is if the developer changes the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
render() { | ||
const { value, disabled, inheritedAttributes } = this; | ||
const ariaLabel = inheritedAttributes['aria-label'] || null; | ||
|
||
return ( | ||
<Host id={this.optionId} class={getIonMode(this)}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking that:
But since I'm not 100% sure if my reasoning is correct and that it's currently not being used, then I removed it. |
||
<button | ||
tabindex="-1" | ||
aria-label={ariaLabel} | ||
class={{ | ||
'picker-opt': true, | ||
'picker-opt-disabled': !!disabled, | ||
averyjohnston marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}} | ||
disabled={disabled} | ||
> | ||
<slot>{value}</slot> | ||
</button> | ||
</Host> | ||
); | ||
} | ||
} | ||
|
||
let pickerOptionIds = 0; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
<!DOCTYPE html> | ||
<html lang="en" dir="ltr"> | ||
<head> | ||
<meta charset="UTF-8" /> | ||
<title>Picker Column Option - Basic</title> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0" /> | ||
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet" /> | ||
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet" /> | ||
<script src="../../../../../scripts/testing/scripts.js"></script> | ||
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script> | ||
</head> | ||
|
||
<body> | ||
<main> | ||
<ion-picker-column-option> my option </ion-picker-column-option> | ||
<ion-picker-column-option aria-label="the best one"> other option </ion-picker-column-option> | ||
</main> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import AxeBuilder from '@axe-core/playwright'; | ||
import { expect } from '@playwright/test'; | ||
import { configs, test } from '@utils/test/playwright'; | ||
|
||
/** | ||
* This behavior does not vary across directions | ||
*/ | ||
configs({ directions: ['ltr'] }).forEach(({ config, title }) => { | ||
test.describe(title('picker column option: a11y'), () => { | ||
test('should not have accessibility violations', async ({ page }) => { | ||
await page.goto(`/src/components/picker-column-option/test/a11y`, config); | ||
|
||
const results = await new AxeBuilder({ page }).analyze(); | ||
|
||
expect(results.violations).toEqual([]); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
<!DOCTYPE html> | ||
<html lang="en" dir="ltr"> | ||
<head> | ||
<meta charset="UTF-8" /> | ||
<title>Picker Column Option - Basic</title> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0" /> | ||
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet" /> | ||
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet" /> | ||
<script src="../../../../../scripts/testing/scripts.js"></script> | ||
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script> | ||
<style> | ||
.grid { | ||
display: grid; | ||
grid-template-columns: repeat(3, minmax(250px, 1fr)); | ||
grid-row-gap: 20px; | ||
grid-column-gap: 20px; | ||
} | ||
|
||
h2 { | ||
font-size: 12px; | ||
font-weight: normal; | ||
|
||
color: #6f7378; | ||
|
||
margin-top: 10px; | ||
margin-left: 5px; | ||
} | ||
|
||
@media screen and (max-width: 800px) { | ||
.grid { | ||
grid-template-columns: 1fr; | ||
padding: 0; | ||
} | ||
} | ||
</style> | ||
</head> | ||
|
||
<body> | ||
<ion-app> | ||
<ion-header translucent="true"> | ||
<ion-toolbar> | ||
<ion-title>Picker Column Option - Basic</ion-title> | ||
</ion-toolbar> | ||
</ion-header> | ||
<ion-content class="ion-padding"> | ||
<div class="grid"> | ||
<div class="grid-item"> | ||
<h2>Default</h2> | ||
<ion-picker-column-option> my option </ion-picker-column-option> | ||
</div> | ||
</div> | ||
</ion-content> | ||
</ion-app> | ||
</body> | ||
</html> |
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.