Skip to content

fix(tags): +n tags for listview#25603

Merged
hughhhh merged 7 commits intoapache:masterfrom
preset-io:n-tags-tooltip
Oct 18, 2023
Merged

fix(tags): +n tags for listview#25603
hughhhh merged 7 commits intoapache:masterfrom
preset-io:n-tags-tooltip

Conversation

@hughhhh
Copy link
Copy Markdown
Member

@hughhhh hughhhh commented Oct 11, 2023

SUMMARY

Fix tooltip to render info about tag in collapse and overall naming even if it's long

Screenshot 2023-10-10 at 11 15 37 PM

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@hughhhh hughhhh changed the title fix tooltip fix(tags): +n tags for listview Oct 11, 2023
@hughhhh hughhhh marked this pull request as ready for review October 13, 2023 02:29
@hughhhh
Copy link
Copy Markdown
Member Author

hughhhh commented Oct 13, 2023

/testenv TAGGING_SYSTEM=True

@hughhhh
Copy link
Copy Markdown
Member Author

hughhhh commented Oct 13, 2023

/testenv up TAGGING_SYSTEM=true

@github-actions
Copy link
Copy Markdown
Contributor

@hughhhh Ephemeral environment spinning up at http://34.221.218.192:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

target="_blank"
rel="noreferrer"
>
{isLongTag ? `${name.slice(0, 20)}...` : name}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we put a const at the top holding this 20 ? I see we're using it in a few places and would be good to just update it once if needed. Or instead of having a isLongTag bool, why not just truncating the name altogether and use that when rendering? if it's a long tag we are always truncating it either way.

) : (
tagElem
);
return tagElem;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have a test asserting we are rendering the "short" version of the tag when needed?

Copy link
Copy Markdown
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

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

lgtm, after fixing the tests it should be fine.

@hughhhh hughhhh merged commit a27a809 into apache:master Oct 18, 2023
@hughhhh hughhhh deleted the n-tags-tooltip branch October 18, 2023 03:39
@github-actions
Copy link
Copy Markdown
Contributor

Ephemeral environment shutdown and build artifacts deleted.

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 First shipped in 3.1.0 labels Mar 8, 2024
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 3.1.0 First shipped in 3.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants