Skip to content

[Twig] Handle aria-* attribute boolean values #1709

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 10, 2024
Merged

[Twig] Handle aria-* attribute boolean values #1709

merged 1 commit into from
Apr 10, 2024

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Apr 9, 2024

Q A
Bug fix? yes
New feature? no
Issues Fix #1701
License MIT

Convert aria-* boolean attributes to their expected string values.

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Apr 9, 2024
kbond
kbond approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

@WebMamba, any thoughts/objections?

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Apr 9, 2024
WebMamba
WebMamba approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Perfect! Sound good to me! Thanks @smnandre

@javiereguiluz
Copy link
Member

Nice bug fix! Thanks Simon.

@javiereguiluz javiereguiluz force-pushed the feat/aria-boolean-attributes branch from d4fe1e0 to 4ac1f61 Compare April 10, 2024 11:16
@javiereguiluz javiereguiluz merged commit 5039e6f into symfony:2.x Apr 10, 2024
@norkunas
Copy link
Contributor

norkunas commented Apr 29, 2024 •
edited
Loading

This is a BC break for us :D aria-valuemin, aria-valuemax and aria-valuenow are number attributes for a progress component.

Example from component:
attributes.defaults({ 'aria-valuemin': value is not null ? 0 : false, 'aria-valuemax': value is not null ? 100 : false, 'aria-valuenow': value is not null ? value : false })

Previously these attributes were hidden and now they just rendered as aria-valuemin="false"

@norkunas norkunas mentioned this pull request Apr 29, 2024
Merged
@smnandre
Copy link
Member Author

smnandre commented Apr 29, 2024 •
edited
Loading

@norkunas could you open a new issue ? (i cannot "move" your comment to a new one)

I'd like to find a coherent solution for both Icon and TwigComponent, fixing the BC (sorry for that)

(updated)

@smnandre
Copy link
Member Author

Sorry did not see the other comment ;)

@smnandre smnandre mentioned this pull request Apr 30, 2024
Merged
kbond added a commit that referenced this pull request Apr 30, 2024
@kbond
bug #1805 [TwigComponent] Fix aria attribute cannot be removed (smnan…
955764b 
…dre)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[TwigComponent] Fix aria attribute cannot be removed

Handle only "true" (the original need) and restore using false to remove an attribute

cf #1709 and #1797

Commits
-------

e3e9825 [TwigComponent] Fix aria attribute cannot be removed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbond kbond kbond approved these changes

@WebMamba WebMamba WebMamba approved these changes

Assignees
No one assigned
Labels
Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TwigComponent][DX] Better support for boolean ARIA attributes (i.e. aria-hidden)
6 participants
@smnandre @javiereguiluz @norkunas @kbond @WebMamba @carsonbot