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

Make links underlined everywhere #20940

Merged
merged 45 commits into from
Jul 12, 2023
Merged

Make links underlined everywhere #20940

merged 45 commits into from
Jul 12, 2023

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jun 27, 2023

Description:

fixes #20706

Review

@sgiehl sgiehl added c: Design / UI For issues that impact Matomo's user interface or the design overall. c: Accessibility When something is not usable for a certain group (eg missing contrast) or devices (eg smartphones). labels Jun 27, 2023
@sgiehl sgiehl added this to the 5.0.0 milestone Jun 27, 2023
@sgiehl sgiehl force-pushed the linkstyles branch 7 times, most recently from 9ae1d61 to b9e50c2 Compare July 4, 2023 16:22
@sgiehl
Copy link
Member Author

sgiehl commented Jul 4, 2023

@bx80 @michalkleiner If you have some time, feel free to already have a look at that one. I hop all links should be underlined by now, but I'm still looking a bit through the changed screenshots to check if I see a link that might still not be underlined. If you spot anything that still needs to be adjusted, let me know.

@@ -262,7 +262,7 @@ public function configureView(ViewDataTable $view)
};
}
} elseif ($view->isViewDataTableId(Evolution::ID)) {
if (!empty($idSite) && Piwik::isUserHasWriteAccess($idSite)) {
if (!empty($idSite) && Piwik::isUserHasWriteAccess($idSite) && $idGoal > GoalManager::IDGOAL_ORDER) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a "bug", where the ecommerce goal title was clickable, referring to an invalid page, as the ecommerce goal isn't editable.

@sgiehl sgiehl force-pushed the linkstyles branch 3 times, most recently from 0d82e3b to d0593df Compare July 7, 2023 13:30
color: @theme-color-text;
white-space: nowrap;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually a small improvement so that the link content › more isn't split onto two lines.

@sgiehl sgiehl force-pushed the linkstyles branch 2 times, most recently from 7d20eba to 18f47f4 Compare July 9, 2023 11:52
@sgiehl sgiehl marked this pull request as ready for review July 9, 2023 17:01
@sgiehl sgiehl added the Needs Review PRs that need a code review label Jul 9, 2023
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

I've looked through every config and dashboard page in the UI and it generally all look really good.

The only things I noticed:

<a> buttons without the btn class shown an underline on hover, whereas <a> buttons with the btn class do not. For example Admin > Personal > Security 'Create new Token' vs 'Turn on two-factor authentication'. Changing .tableActionBar a &:hover to text-decorated: none seemed to resolve this but I'm not sure if this is used elsewhere for non-buttons?
Screenshot_20230710_130445

The site selector and period selector buttons don't underline on hover as expected, but the check for update button does, it seems to be the span not the button that has the underline.
Screenshot_20230710_130535

The Admin > Measurables > MTM panel bottom links have the icons underlined along with the link. This is quite a minor thing, so feel free to ignore 🙂
Screenshot_20230710_130404

@sgiehl
Copy link
Member Author

sgiehl commented Jul 10, 2023

@bx80 All of those misplaced underlines where actually already the case before this PR, but I'll try to push some fixes.

@sgiehl sgiehl force-pushed the linkstyles branch 3 times, most recently from 5ac777c to 3eaabf7 Compare July 10, 2023 13:57
@sgiehl
Copy link
Member Author

sgiehl commented Jul 10, 2023

Everything should be updated now.

@sgiehl sgiehl requested a review from bx80 July 10, 2023 14:08
@michalkleiner
Copy link
Contributor

I'll have a quick look today as well

Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

I've checked through page in both admin and dashboard, it's all looking good 👍

@michalkleiner
Copy link
Contributor

We shouldn't be underlying the space between the icon and the text of the link, it should use margin-right to create the space.

space underline
space underline in MTM buttons

@sgiehl sgiehl merged commit eed6ea9 into 5.x-dev Jul 12, 2023
6 of 20 checks passed
@sgiehl sgiehl deleted the linkstyles branch July 12, 2023 14:40
@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. and removed Needs Review PRs that need a code review labels Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Accessibility When something is not usable for a certain group (eg missing contrast) or devices (eg smartphones). c: Design / UI For issues that impact Matomo's user interface or the design overall. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create accessible hyperlinks
3 participants