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

Fixed upf-stat__label issue #348

Merged
merged 4 commits into from
Dec 15, 2023
Merged

Fixed upf-stat__label issue #348

merged 4 commits into from
Dec 15, 2023

Conversation

OwenCoogan
Copy link
Contributor

What does this PR do?

It fixes the display of upf stat label

Related to: #
https://linear.app/upfluence/issue/ENG-2079/creator-media-stats-have-bad-alignment-in-sidepanel

What are the observable changes?

Fixes display issues that emerged with this commit. A condition is added in order to switch classes when a tooltip is present.

Good PR checklist

  • Title makes sense
  • Is against the correct branch
  • Only addresses one issue
  • Properly assigned
  • Added/updated tests
  • Added/updated documentation
  • Migrated touched components to Glimmer Components
  • Properly labeled

@OwenCoogan OwenCoogan requested a review from a team December 14, 2023 10:42
@OwenCoogan OwenCoogan self-assigned this Dec 14, 2023
@OwenCoogan OwenCoogan requested review from phndiaye, Miexil and aprentout and removed request for a team December 14, 2023 10:42
Copy link

linear bot commented Dec 14, 2023

@@ -29,10 +29,10 @@
</span>

{{#if label}}
<span class="upf-stat__label">
<span class={{concat 'upf-stat__label' (if tooltip '--with-tooltip')}}>
Copy link
Member

Choose a reason for hiding this comment

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

use double quotes here :)

Copy link
Member

Choose a reason for hiding this comment

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

and maybe
if tooltip "upf-stat__label--with-tooltip" "upf-stat__label"
is clearer, but debatable 🤷 ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

or
class="upf-stat__label{{if tooltip "--with-tooltip"}}" :trollface:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed for your solution @Miexil

Copy link
Contributor

@aprentout aprentout left a comment

Choose a reason for hiding this comment

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

Otherwise lgtm

@@ -29,10 +29,10 @@
</span>

{{#if label}}
<span class="upf-stat__label">
<span class={{concat 'upf-stat__label' (if tooltip '--with-tooltip')}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

or
class="upf-stat__label{{if tooltip "--with-tooltip"}}" :trollface:

display: flex;
gap: var(--spacing-px-6);
align-items: center;
color: @color-text-light;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Replace @color-text-light with var(--color-gray-500)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep fixed

@OwenCoogan OwenCoogan merged commit 44f77f6 into master Dec 15, 2023
3 checks passed
@OwenCoogan OwenCoogan deleted the oc/ENG-2079 branch December 15, 2023 09:27
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.

5 participants