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

fix: Improve link preview image display #17100

Merged
merged 4 commits into from
Mar 19, 2024
Merged

fix: Improve link preview image display #17100

merged 4 commits into from
Mar 19, 2024

Conversation

atomrc
Copy link
Contributor

@atomrc atomrc commented Mar 19, 2024

Description

Screenshots/Screencast (for UI changes)

Before

image

After

image

Checklist

  • PR has been self reviewed by the author;
  • Hard-to-understand areas of the code have been commented;
  • If it is a core feature, unit tests have been added;

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.15%. Comparing base (1462daf) to head (72b5e71).
Report is 1 commits behind head on dev.

❗ Current head 72b5e71 differs from pull request most recent head 3e88b06. Consider uploading reports for the commit 3e88b06 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##              dev   #17100   +/-   ##
=======================================
  Coverage   46.15%   46.15%           
=======================================
  Files         747      747           
  Lines       24548    24548           
  Branches     5610     5610           
=======================================
  Hits        11329    11329           
  Misses      11786    11786           
  Partials     1433     1433           

@@ -32,7 +32,8 @@
}

.link-preview-image-container {
width: @link-preview-height;
min-width: @link-preview-height;
height: @link-preview-height;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's change the variable to rem, the reason height wasn't fixed was for larger font sizes.

80px would be 5rem, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's why we had this strange : width: @...-height :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using rem sounds like a nice solution 👍

Copy link
Contributor

@V-Gira V-Gira left a comment

Choose a reason for hiding this comment

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

Let's see how that looks :)

@@ -18,7 +18,7 @@
*/

// variables
@link-preview-height: 88px;
@link-preview-height: 5.5rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I read 80px for some reason, good on ya for double checking :D

@atomrc atomrc enabled auto-merge (squash) March 19, 2024 09:56
@atomrc atomrc merged commit f106a01 into dev Mar 19, 2024
10 checks passed
@atomrc atomrc deleted the runfix/link-preview branch March 19, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants