Fix link and text selection in budget header #4825
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
javierm
changed the title
javierm
changed the title
javierm
force-pushed
the
fix_selecting_text_in_budget_header
branch
from
b41f0e8
to
25ba7fb
Compare
While reviewing commit 7702b55, I forgot to test whether selecting text in the budget header or clicking its link worked properly. The negative index (-5) meant it was impossible to select its text or click on its link. The good news is the pseudoelement with a negative index (-1) is considered a child of the .budget-header element, so having a negative index will cause the pseudoelement to be render behind the content of the .budget-header element but in front of the background of the .budget-header element. This is exactly what we want. Originally, we didn't have a z-index in the .budget-header element, meaning the pseudoelement was rendered behind the background of the .budget-header element, meaning both backgrounds were visible. This was OK when the background was a plain color, but it wasn't when the background was an image. To stress the fact that the budget header is only affected when we use an image, I'm also moving the code inside the `.with-background-image` selector, although it would be interesting to check whether it's a good idea to add `z-index: 0` to the `full-width-background` mixin.
javierm
force-pushed
the
fix_selecting_text_in_budget_header
branch
from
25ba7fb
to
bfe6c18
Compare
taitus
approved these changes
May 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Visual Changes
Before these changes
After these changes
Notes
While reviewing commit 7702b55, I forgot to test whether selecting text in the budget header and clicking its link worked properly.
The negative index (-5) meant it was impossible to select its text or click on its link.
The good news is the pseudoelement with a negative index (-1) is considered a child of the .budget-header element, so having a negative index will cause the pseudoelement to be render behind the content of the .budget-header element but in front of the background of the .budget-header element.
This is exactly what we want.
Originally, we didn't have a z-index in the .budget-header element, meaning the pseudoelement was rendered behind the background of the .budget-header element, meaning both backgrounds were visible. This was OK when the background was a plain color, but it wasn't when the background was an image.
To stress the fact that the budget header is only affected when we use an image, I'm also moving the code inside the
.with-background-imageselector, although it would be interesting to check whether it's a good idea to addz-index: 0to thefull-width-backgroundmixin.