Skip to content

Improve narrow YouTube Click to Load placeholders #422

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

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

kzar
Copy link
Contributor

@kzar kzar commented Apr 13, 2023

When the YouTube Click to Load placeholders are very narrow, the
information text takes up too much space and some of the other
elements are lost. Let's tweak narrow placeholders, to hide that
information text and move the "Learn more" link. This isn't the best
approach (using @container queries[1] might be better), but this code
is going to be refactored shortly anyway.

1 - https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Container_Queries

@kzar kzar requested a review from franfaccin April 13, 2023 14:27
@kzar kzar requested a review from ladamski as a code owner April 13, 2023 14:27
@kzar kzar force-pushed the fix-ctl-youtube-placeholders-p8 branch from 1395c62 to 23bf8f9 Compare April 13, 2023 14:27
@kzar kzar marked this pull request as draft April 13, 2023 15:06
@kzar kzar force-pushed the fix-ctl-youtube-placeholders-p8 branch from 23bf8f9 to cd72c13 Compare April 13, 2023 16:09
@kzar kzar marked this pull request as ready for review April 13, 2023 16:10
@kzar kzar force-pushed the fix-ctl-youtube-placeholders-p8 branch 4 times, most recently from def7e31 to 96c1d3a Compare April 17, 2023 11:44
Copy link
Contributor

@franfaccin franfaccin left a comment

Choose a reason for hiding this comment

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

I think you're handling well these changes. I wonder if it would be best to create a new text in the config instead of moving the "learn more" around. But there wouldn't be much gain either way, and like you said, we will likely refactor this code soon, so it should be fine to leave it like this to unblock you. I left some other general comments, ping me if you have any questions :)

@kzar
Copy link
Contributor Author

kzar commented Apr 17, 2023

I wonder if it would be best to create a new text in the config instead of moving the "learn more" around.

Yea, you're right there. Moving the string around like this isn't correct really, and with the new preview sub-heading string the link is included. I figured this is pragmatic for now though, in particular since we're doing the same thing when adding this particular link originally - we append it to a div with the text content of hoverTextBody + ' '.

@kzar kzar force-pushed the fix-ctl-youtube-placeholders-p8 branch from 96c1d3a to 82a4010 Compare April 17, 2023 14:36
When the YouTube Click to Load placeholders are very narrow, the
information text takes up too much space and some of the other
elements are lost. Let's tweak narrow placeholders, to hide that
information text and move the "Learn more" link. This isn't the best
approach (using @container queries[1] might be better), but this code
is going to be refactored shortly anyway.

1 - https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Container_Queries
@kzar kzar force-pushed the fix-ctl-youtube-placeholders-p8 branch from 82a4010 to c83b993 Compare April 18, 2023 14:28
@kzar kzar merged commit bfd0d9a into duckduckgo:main Apr 18, 2023
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.

2 participants