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

fix(@clayui/core): Adapt snapshots #5894

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

veroglez
Copy link
Member

Hi guys!

I am sending you the fix for this issue https://liferay.atlassian.net/browse/LPD-42404

Thanks in advance! 😄

@matuzalemsteles
Copy link
Member

Hi @veroglez, I think this could break other use cases in DXP taking into context when we added this it was to fix the Forms use case #4782. I think the best thing here would be to add an API to disable this something like truncate would be safer because we've had it for over 2 years so there are probably more use cases expecting this to be truncated. What do you think?

@veroglez
Copy link
Member Author

veroglez commented Nov 20, 2024

Hi @veroglez, I think this could break other use cases in DXP taking into context when we added this it was to fix the Forms use case #4782. I think the best thing here would be to add an API to disable this something like truncate would be safer because we've had it for over 2 years so there are probably more use cases expecting this to be truncated. What do you think?

Hi @matuzalemsteles! We discussed this issue on Slack in this thread and we came to the conclusion that the truncated text is an accessibility issue. But I would like to mention @marcoscv-work to clarify this.

@marcoscv-work
Copy link
Member

Hi @veroglez, I think this could break other use cases in DXP taking into context when we added this it was to fix the Forms use case #4782. I think the best thing here would be to add an API to disable this something like truncate would be safer because we've had it for over 2 years so there are probably more use cases expecting this to be truncated. What do you think?

Hi @matuzalemsteles! We discussed this issue on Slack in this thread and we came to the conclusion that the truncated text is an accessibility issue. But I would like to mention @marcoscv-work to clarify this.

Yeah, truncate-text on navigation items is an issue in itself. We fixed this in some places during the BofA big accessibility effort, so from my side, we can directly remove the class, and we don't need to offer an alternative.
If forms or any other part of the product/team considers this an issue, you guys can directly refer to me.

For more detail, we have an internal document called "About truncate text and text-truncate class" Sept 17, 2024

@ethib137
Copy link
Member

@marcoscv-work and @veroglez I think it's reasonable to remove the class, but we should still account for the case that forms raised. Currently the nav item text will wrap, but only on whitespace. If we are removing text-truncate then it probably makes sense to allow it to break words in order to wrap and not overflow the container and even the screen. What do you all think?

@marcoscv-work
Copy link
Member

@marcoscv-work and @veroglez I think it's reasonable to remove the class, but we should still account for the case that forms raised. Currently the nav item text will wrap, but only on whitespace. If we are removing text-truncate then it probably makes sense to allow it to break words in order to wrap and not overflow the container and even the screen. What do you all think?

Yes! We probably didn't focus on and review accessibility too much in 2022.
The specific case for Forms is very interesting because they are using the navigation as a Title of the form when is not possible to use the component to navigate, so they shouldn't be using the navigation:

image

They should use a title. @drakonux already proposed how to mix the titles in control menu, but right now, and as an example, they could use any of the existing titles in page details, here is an example of file detail in Documents and Media:
image

As a final conclusion, I find it interesting that we implemented this text truncate for a misuse that Forms makes of a navigation component :-)

@marcoscv-work
Copy link
Member

Issue created:
https://liferay.atlassian.net/browse/LPD-42665
(Waiting for forms validation)

cc @veroglez @ethib137 @matuzalemsteles

@veroglez
Copy link
Member Author

Issue created: https://liferay.atlassian.net/browse/LPD-42665 (Waiting for forms validation)

cc @veroglez @ethib137 @matuzalemsteles

Thanks @marcoscv-work! so can we move this PR forward?

@marcoscv-work
Copy link
Member

it looks safe to me, let's wait for @matuzalemsteles and @ethib137

@ethib137
Copy link
Member

I'm okay with removing it, since it looks like this was not reported by a customer.

I think the fix should involve removing a bit more code though. I would expect it to be more of a revert of this commit than just removing a class.

@matuzalemsteles what do you think?

@matuzalemsteles
Copy link
Member

I think the fix should involve removing a bit more code though. I would expect it to be more of a revert of this commit than just removing a class.

Yeah I agree reverting this commit is better in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants