-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Correctly render dag tags when there are MAX_TAGS + 1 tags
#50669
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
Conversation
shubhamraj-git
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
One comment : I can see 2 commas in case of 5 tags.
|
@shubhamraj-git i realised that and pushed a fix for it now, updated the images too. |
|
What are you trying to solve here? The reason I had that logic before is because "+1 more" is just as long as just showing that one extra tag. So we should only use the overflow once it actually saves us space |
|
How much of a pain would it be to show the full list vertically stacked on hover over the + element? Suppose we can address in a subsequent pull but jw because in this case we have no access to the full list of tags once truncated. On a tooltip I mean. |
@bbovenzi the current behaviour without this fix is a squash of MAX_LEN'th element in the tags. My tags: I guess we can either show a "+n more" in a more generic sense which is for n > MAX_LEN or just do something more better. We cannot have the last 2 tags concatenate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I think as Brent mentioned we need a little bit of both. There's no point showing +1 more we might as well show the last tag at this point.
But indeed in that particular scenario we need a correct separator between the max and max + 1 tag. (not a concatenation the two tags as you highlighted)
|
There's one thing though -- "+1 more" will be much much shorter than having a tag of max_len, defined as So should we have some logic that something in between? Show the min of the two? |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed +1 more will be shorter if the extra tag name is really long, but we don't have logic based on the tag length at all. And if we do that for the latest tag should also do it for previous tags.
I wouldn't bother going down that rabbit hole for now.
|
Definitely yes |
|
@pierrejeambrun / @bbovenzi how do you suggest we proceed with this one? |
|
I would only solve the current case of a missing |
MAX_TAGS + 1 tags
|
Sounds fair, yeah. I have handled that case and pushed a fix for it. Also updated PR desc and title. |
…50669) * Correctly handle rendering dag tags when tags are 1 more than max * Correctly handle rendering dag tags when tags are 1 more than max * fixing tests * comments from pierre
…50669) * Correctly handle rendering dag tags when tags are 1 more than max * Correctly handle rendering dag tags when tags are 1 more than max * fixing tests * comments from pierre

closes: #50639
Previously, when the item count was maxItems + 1, the last visible item and "+1 more" were rendered without a separator, leading to formatting issues. This fixes the logic to include the separator in this specific edge case.
For 3 tags:
For 4 tags:
FOr 5 tags:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.