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

Qoldev 339 outline hovering underline #866

Closed

Conversation

ghazal-salehi
Copy link
Contributor

@ghazal-salehi ghazal-salehi changed the base branch from master to release-v4.2.0 April 2, 2023 23:41
@ghazal-salehi ghazal-salehi self-assigned this Apr 2, 2023
@ghazal-salehi
Copy link
Contributor Author

ghazal-salehi commented Apr 5, 2023

I had to update this PR as the pb: 3px was impacting other links like nav items. The issue was caused because the standard links did not have any height applied to them. By assigning display value they would. This change also fixes https://ssq-qol.atlassian.net/browse/QOLDEV-322 (The logo outline issue in Safari)

@ghazal-salehi ghazal-salehi requested a review from ThrawnCA April 5, 2023 00:33
@RossWebsterWork
Copy link

It seems like a nice fix, but I think making all links display as an inline-block may have unintended layout effects, such as for links which go over multiple lines.
I believe @ThrawnCA encountered this before in #749

@@ -19,6 +19,7 @@
// &.hover {
// background: none;
// }
display: inline-block;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be applied globally. It has side effects on line spacing and wrapping.

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 think increasing line heights is inevitable to fix this issue, because the style lacks some vertical spacing to fit in both outline and underline. I excluded digital dashboard from the change.

@ghazal-salehi
Copy link
Contributor Author

The impact on multi-line links looks like this:
Example on small size screen: https://oss-uat.clients.squiz.net/disability/legal-and-rights/advocacy

Before:

image

After:

image

I agree it is different, but does not look broken. I can ask Bec's opinion as well.

Such change would fix the outline for the logo in FireFox and Safari as well:

Before:
image

After:
image

I also noticed other in the system that did not have this issue (outline covering underline), already have display: inline-block, like nav items and qg-index-item.
image

image

https://www.qld.gov.au/disability/legal-and-rights/advocacy
https://www.qld.gov.au/disability

I checked Carl's PR, .pagination and .graph were excluded. I tested pagination has it's own class (page-link) to apply display: block to .
pagination: https://oss-uat.clients.squiz.net/search?query=test&num_ranks=10&tiers=off&collection=qld-gov&profile=qld&scope=&page=1&start_rank=1

I excluded #ict-tabcontainer a from the added style.
https://oss-uat.clients.squiz.net/digitalprojectsdashboard/dashboard?SQ_VARIATION_102100=0

@ghazal-salehi
Copy link
Contributor Author

In links it is noticeable that it is rendered something in the middle, so not exactly covered, and not spaced enough. https://www.qld.gov.au/search?query=test&num_ranks=10&tiers=off&collection=qld-gov&profile=qld

Before:
image

After:
image

@ghazal-salehi ghazal-salehi requested a review from ThrawnCA April 6, 2023 05:11
Copy link
Contributor

@ThrawnCA ThrawnCA left a comment

Choose a reason for hiding this comment

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

Restricting inline-block with a blacklist isn't enough. It shouldn't be the default.

@ghazal-salehi
Copy link
Contributor Author

Closing the PR. This change needs to be done after reviewing font sizes by Bec. We can do this by using styles like outline-offset.

@elvishu elvishu deleted the QOLDEV-339-outline-hovering-underline branch July 26, 2024 02:19
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.

3 participants