-
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(checkbox): display as block when justify or alignment properties are defined #29783
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…box wrapper" This reverts commit 7365fb3.
@@ -36,7 +36,7 @@ configs({ directions: ['ltr'] }).forEach(({ title, config, screenshot }) => { | |||
font-size: 310%; | |||
} | |||
</style> | |||
<ion-checkbox justify="start" checked>Checked</ion-checkbox> |
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.
The justify
property does not need to be on this test as all it does is force the Checkbox to be 100% width when it isn't necessary.
@@ -72,7 +72,9 @@ | |||
flex-grow: 1; | |||
|
|||
align-items: center; | |||
justify-content: space-between; |
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.
This is required so that the user can still do the following, with a space between the label and checkbox:
<ion-checkbox style="width: 100%">Label</ion-checkbox>
*/ | ||
configs().forEach(({ title, screenshot, config }) => { | ||
test.describe(title('checkbox: label'), () => { | ||
test.describe('checkbox: start placement', () => { | ||
test('should render a start justification with label in the start position', async ({ page }) => { | ||
await page.setContent( | ||
` | ||
<ion-checkbox label-placement="start" justify="start" style="width: 200px">Label</ion-checkbox> |
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.
An explicit width is no longer needed since using justify
sets display: block
which takes up the full-width of its container.
Long Label Long Label Long Label Long Label Long Label Long Label | ||
</ion-checkbox> | ||
`, | ||
config | ||
); | ||
|
||
const checkbox = page.locator('ion-checkbox'); | ||
await expect(checkbox).toHaveScreenshot(screenshot(`checkbox-long-label`)); | ||
await expect(checkbox).toHaveScreenshot(screenshot(`checkbox-label-start-justify-start-long-label`)); |
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.
Renamed this test to match the other prefixes in this describe block.
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.
These width differences are expected since removing the explicit width: 200px
makes the checkbox take up the full width with justify
defined.
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.
These width differences are expected since removing the explicit width: 200px
makes the checkbox take up the inline width without justify
defined.
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.
This was renamed to checkbox-label-start-justify-start-long-label
4f6b241
to
c620298
Compare
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.
LGTM
Issue number: internal
What is the current behavior?
When adding the
justify
oralignment
property to anion-checkbox
it does not change the design because the checkbox is displayed inline.What is the new behavior?
display
property toblock
when thejustify
oralignment
property is set.justify-content
style tospace-between
so that a checkbox withwidth: 100%
set withoutjustify
oralignment
set will still have the same defaultlabel
e2e test to remove the explicit width as setting the property will make them full-widthlabel
e2e test of labels that do not havejustify
set but usewidth: 100%
to ensure they are working as expected without itDoes this introduce a breaking change?
Other information