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

[4.0] Underline links by default #33808

Closed
wants to merge 5 commits into from
Closed

[4.0] Underline links by default #33808

wants to merge 5 commits into from

Conversation

ciar4n
Copy link
Contributor

@ciar4n ciar4n commented May 11, 2021

Pull Request for Issue #33629 .

Summary of Changes

Underline links by default

Testing Instructions

Actual result BEFORE applying this Pull Request

By default links are not underlined

Expected result AFTER applying this Pull Request

By default links are underlined

Documentation Changes Required

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels May 11, 2021
@PhilETaylor

This comment was marked as abuse.

@ciar4n
Copy link
Contributor Author

ciar4n commented May 11, 2021

I'm not a fan myself but I'm assuming this is a AA a11y requirement?

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@ciar4n
Copy link
Contributor Author

ciar4n commented May 11, 2021

@PhilETaylor Agreed. Underline removed from icons with 2a68f4c

Im sure our resident expert will be along to confirm or deny this is an accessibility thing :)

@brianteeman Willing or not, I'm assigning that grand title to you 😉

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

it was the removal of the flex-grow that caused the problem i think

@ghost
Copy link

ghost commented May 12, 2021

I have tested this item ✅ successfully on 2a68f4c

If there is time for finetuning:

image


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33808.

@ceford
Copy link
Contributor

ceford commented May 12, 2021

I have tested this item ✅ successfully on 2a68f4c

Many years ago I read somewhere that links are supposed to be visually distinguishable from adjacent text. So this fits the bill although the underline is barely visible and I agree with others it is a bit yucky. Whatever happened to the default ancient blue/purple underlined scheme?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33808.

@richard67
Copy link
Member

If there is time for finetuning:

image

@ciar4n Do you think you could fix that, too, the underlined space between the link and any appended language badge?

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33808.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 12, 2021
@richard67 richard67 added this to the Joomla 4.0 milestone May 12, 2021
@brianteeman
Copy link
Contributor

Please remove RTC the issue reported by @sandramay0905 needs to be fixed

@richard67 richard67 removed the RTC This Pull Request is Ready To Commit label May 12, 2021
@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone May 12, 2021
@richard67
Copy link
Member

Back to pending. See previous comments.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33808.

@richard67 richard67 added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label May 12, 2021
@brianteeman
Copy link
Contributor

Is it possible to only show the underline on the text and not the whitespace between the icon and the text?

Probably something like adding display: contents; to the link as the problem comes from the whitespace (newlines and tabs)

@richard67
Copy link
Member

Is this PR here still needed or useful now as #33811 has been merged?

@PhilETaylor

This comment was marked as abuse.

@chmst
Copy link
Contributor

chmst commented May 14, 2021

Underline together with icon is really too much and not required. A user must see what is a link but the icon is sufficient for that.

@PhilETaylor

This comment was marked as abuse.

@ciar4n
Copy link
Contributor Author

ciar4n commented May 14, 2021

Is this PR here still needed or useful now as #33811 has been merged?

If #33811 is the accepted solution then maybe this PR is overkill as it makes links underlined by default where #33811 only underlines links in the cpanel modules?

@ciar4n
Copy link
Contributor Author

ciar4n commented May 14, 2021

@sandramay0905 Could you detail for me how to replicate? ....

image

@ghost
Copy link

ghost commented May 15, 2021

@ciar4n You need a multilanguage site, you can see it on Menu Dashboard for the menus having Home set.

@ciar4n ciar4n closed this May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants