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: adjust active element visibility in nclistitem #4158

Merged
merged 1 commit into from Aug 23, 2023

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented May 25, 2023

Before After
image image

todo:

problem1 problem2
image image

@szaimen szaimen added 2. developing Work in progress 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities and removed 2. developing Work in progress labels May 25, 2023
Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

We also need to take care of the counter and the unread dot for mail

Screenshot 2023-05-25 at 17 47 05

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 25, 2023
@szaimen szaimen force-pushed the enh/noid/nclistitem-visilibty branch from 23e3d5f to 6ba9ff4 Compare May 25, 2023 15:41
@szaimen
Copy link
Contributor Author

szaimen commented May 25, 2023

@marcoambrosini can you help me to get an active element as example into the docs? This would make testing this a bit easier :)

@nimishavijay
Copy link

Looks good in the screenshot, but as Marco said the dot needs to be adjusted, it can be changed to white :)

@jancborchardt
Copy link
Contributor

Looks good, and yep as @nimishavijay said switching the dot to white on the active element is good, so it’s symmetric to the "message for you" counter bubble color.

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

looks good, thank you!

contrast of "talk icon" should't be increased because it is only a decorative icon

Identify each graphic on the page that includes information required for understanding the content (i.e. excluding graphics which have visible text for the same information, or are decorative)
from https://www.w3.org/WAI/WCAG21/Understanding/non-text-contrast.html

@marcoambrosini
Copy link
Contributor

@szaimen just add the .active class to the outer element :)

@szaimen szaimen force-pushed the enh/noid/nclistitem-visilibty branch from 4a7ad19 to f1e5562 Compare May 30, 2023 13:14
@szaimen
Copy link
Contributor Author

szaimen commented May 30, 2023

@szaimen just add the .active class to the outer element :)

thanks for the pointer! Works great! :)

@JuliaKirschenheuter
Copy link
Contributor

JuliaKirschenheuter commented Jun 1, 2023

Probably i haven't understood a problem, but why we could't add an .active class to three dots icons if an element is active one?

@szaimen
Copy link
Contributor Author

szaimen commented Jun 1, 2023

Probably i haven't understood a problem, but why we could't add an .active class to three dots icons if an element is active one?

I fear the problem is that this should probably be done automatically for embedded components? Also there is not active state for nccounterbubble for example afaik...

@marcoambrosini

This comment was marked as resolved.

@szaimen

This comment was marked as resolved.

@marcoambrosini

This comment was marked as resolved.

@szaimen
Copy link
Contributor Author

szaimen commented Jun 20, 2023

@JuliaKirschenheuter would you mind taking this over? I am unfortunatley techincally stuck on how to proceed here.

@marcoambrosini
Copy link
Contributor

I can take over :)

@marcoambrosini marcoambrosini self-assigned this Jun 21, 2023
@szaimen
Copy link
Contributor Author

szaimen commented Jun 21, 2023

I can take over :)

Thanks a lot Marco! 💙🙏

@marcoambrosini
Copy link
Contributor

I just reworked a bit. @skjnldsv @raimund-schluessler @susnux do you think this would cover all the use cases? Maybe we should also include a prop to force-primary style in the cases where the element is not a router link?

Screenshot 2023-08-11 at 13 46 55

@szaimen
Copy link
Contributor Author

szaimen commented Aug 11, 2023

I just reworked a bit. @skjnldsv @raimund-schluessler @susnux do you think this would cover all the use cases?

The screenshot looks good to me :)

Can you commit your changes? They arent yet afaics

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good design-wise! :)

@marcoambrosini

This comment was marked as off-topic.

@skjnldsv
Copy link
Contributor

we should also include a prop to force-primary style

an active prop would be nice sure :)

@skjnldsv

This comment was marked as resolved.

@szaimen
Copy link
Contributor Author

szaimen commented Aug 16, 2023

Tested in the docs and on hover the 3-dots button looks like it does not fit the contrast requirements:
image

@jancborchardt
Copy link
Contributor

@szaimen yeah, the icon needs to invert then to the same as text color (white in the case of default blue primary color).

@skjnldsv skjnldsv changed the title adjust active eleement visibility in nclistitem fix: adjust active element visibility in nclistitem Aug 17, 2023
@marcoambrosini
Copy link
Contributor

That is because the element is not activated unless the route is matched. I'm adding a "forceactive" prop now

Copy link
Contributor Author

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Tested and seem to work however the nclistitem compact mode looks a bit strange now. Not sure though if this is caused by this change:

This PR: image

master:

image

Signed-off-by: Marco <marcoambrosini@icloud.com>
@marcoambrosini
Copy link
Contributor

Should be fixed now @szaimen!

Copy link
Contributor Author

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

LGTM! :)

@szaimen szaimen marked this pull request as ready for review August 23, 2023 11:57
@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 23, 2023
@skjnldsv skjnldsv merged commit 2412bbf into master Aug 23, 2023
17 checks passed
@skjnldsv skjnldsv deleted the enh/noid/nclistitem-visilibty branch August 23, 2023 14:39
@skjnldsv skjnldsv added the bug Something isn't working label Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants