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(description-list): Adding option and logic to hide/show items in lists - FRONT-3895 #2821

Merged
merged 22 commits into from
Apr 12, 2023

Conversation

planctus
Copy link
Contributor

@planctus planctus commented Mar 31, 2023

as requested standaone links, inline links, taxonomy list items can be limited by setting a common parameter when including the template.
.
A control has been added in the story as a range, by default the vertical example is having collapsed lists while the horizontal doesn't.

@github-actions
Copy link

github-actions bot commented Mar 31, 2023

this.lists.forEach((list) => {
if (list.children && list.children.length > this.visibleItems) {
this.showHide(list.children);
const button = document.createElement('a');
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it be better to use a real button instead of a link (without href)?

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 changed this in the last commit, it is because of the styles that the buttons have, that were a bit problematic and would sure requiremore iterations on the design side to tweak it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, to be seen if that doesn't raise accessibility issues later (having a link without href is usually a bad practice)
Also the css has to be updated in this case, to have the hand cursor on hover (it isn't added if there is no href)

Copy link
Contributor Author

@planctus planctus Apr 11, 2023

Choose a reason for hiding this comment

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

i added the href.. ;) a link is still valid without an href but without it we would not be able to make the keyboard navigation work as expected without additional coding, so i simply added it

planctus and others added 2 commits April 11, 2023 14:42
…on-list.html.twig

Co-authored-by: Romain Emery <emery.romain@gmail.com>
…on-list.html.twig

Co-authored-by: Romain Emery <emery.romain@gmail.com>
@emeryro
Copy link
Contributor

emeryro commented Apr 11, 2023

Now that we can focus the link, I noticed that it is displayed wrongly, with the click area going outside of the the text.
image

@planctus
Copy link
Contributor Author

thanks @emeryro, the focusable area should be ok now

emeryro
emeryro previously approved these changes Apr 11, 2023
@github-actions
Copy link

@github-actions github-actions bot temporarily deployed to pull request April 12, 2023 10:43 Inactive
@emeryro emeryro merged commit c14ffdc into v3.8.0-dev Apr 12, 2023
@emeryro emeryro deleted the FRONT-3895-Description-list-visible-items branch April 12, 2023 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants