fix: Truncated synchronize check-boxes should have tooltips #4233 #6954

ciiay · 2021-08-11T02:41:27Z

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

…4233 Signed-off-by: ciiay <yicai@redhat.com>

codecov · 2021-08-11T02:55:41Z

Codecov Report

Merging #6954 (5cf5f63) into master (0dcac9a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6954   +/-   ##
=======================================
  Coverage   40.98%   40.98%           
=======================================
  Files         158      158           
  Lines       21305    21305           
=======================================
  Hits         8731     8731           
  Misses      11334    11334           
  Partials     1240     1240

Impacted Files	Coverage Δ
util/settings/settings.go	`47.38% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dcac9a...5cf5f63. Read the comment docs.

jsoref · 2021-08-11T18:40:18Z

ui/src/app/applications/components/application-sync-panel/application-sync-panel.tsx

+                                                    const contentStart = resKey.substr(0, Math.floor(resKey.length / 2));
+                                                    let contentEnd = resKey.substr(-Math.floor(resKey.length / 2));
+                                                    // Fix Latin letters being rendered left to right on ellipsis with direction set to rtl
+                                                    const indexOfFirstLetter = /[a-z]/i.exec(contentEnd).index;
+                                                    contentEnd = contentEnd.slice(indexOfFirstLetter);


I'm puzzled by this.
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/

Afaict, this regex: /[-0-9a-zA-Z.]{0,253}/ allows more things than kubernetes does.

Does ArgoCD support translations at all?

Thanks for the review. Could you elaborate what translation did you mean?

Code in line 178 and line 179 is to avoid contentEnd string which starts with special characters such as /, - and . being placed to the rightmost of contentEnd string when rendered with direction property set to rtl value. For example, if contentEnd is /abcd/argo-cd/test/example, without the fix it would be rendered as ...cd/argo-cd/test/example/.

I'm mostly interested in line 177.

RTL relates to Arabic and Hebrew rendering (namely that logical elements are rendered Right-to-Left). Typically this only happens if you have a "localization" of an application, typically performed by translating the user interface.

For example:
https://www.google.co.il/?hl=he

Given that as far as I can tell, it is not possible to view ArgoCD in Arabic or Hebrew, and given that kubernetes resources can't have non ascii (~Latin) characters, a reference to RTL seems out of place.

https://developer.mozilla.org/en-US/docs/Glossary/rtl

Simply put, do you have an example of RTL actually breaking? If so, a picture here would be helpful.

Sure, I added this only because I observed the weird extra /, - and . at the end of the contentEnd string. Here's an example with out the two lines code. Note that the content-end attribute has the value of /CustomResourceDefinition/..., and then the / got moved to the end of the contentEnd of the rendered resource string. Did some research on this issue and it turned out /, - and . are considered Latin characters which will be put in the rightmost when use direction of right-to-left. So I made the change to make the contentEnd string starts with the first English letter instead the original string.

…4233-add-tooltips-for-truncated-check-box-label

jsoref · 2021-08-17T19:48:37Z

ui/src/app/applications/components/application-sync-panel/application-sync-panel.scss

+        label::after {
+            content: attr(content-end);
+            text-overflow: clip;
+            direction: rtl;


This direction: rtl is doing a lot of work and needs a comment

jsoref · 2021-08-17T19:50:05Z

ui/src/app/applications/components/application-sync-panel/application-sync-panel.tsx

@@ -172,9 +172,27 @@ export const ApplicationSyncPanel = ({application, selectedResource, hide}: {app
                                                .filter(item => !item.hook)
                                                .map((item, i) => {
                                                    const resKey = nodeKey(item);
+                                                    const contentStart = resKey.substr(0, Math.floor(resKey.length / 2));
+                                                    let contentEnd = resKey.substr(-Math.floor(resKey.length / 2));
+                                                    // Fix Latin letters being rendered left to right on ellipsis with direction set to rtl


@ciiay reached out to me and explained that this is tied to the css change.

A comment in both spots explaining that it's relying on the direction change to place the ellipsis where we want it would help.

And then this comment after it can explain "and because we've direction swapped, we now trip on strong LTR characters, so we need to do a bit of extra work for them".

ciiay · 2021-08-17T19:50:19Z

ui/src/app/applications/components/application-sync-panel/application-sync-panel.scss

+        label::after {
+            content: attr(content-end);
+            text-overflow: clip;
+            direction: rtl;


Adding a comment here after direction: rtl; as // This is for ellipsis in the middle instead of the end of resource path.

ciiay · 2021-08-17T19:52:22Z

ui/src/app/applications/components/application-sync-panel/application-sync-panel.tsx

@@ -172,9 +172,27 @@ export const ApplicationSyncPanel = ({application, selectedResource, hide}: {app
                                                .filter(item => !item.hook)
                                                .map((item, i) => {
                                                    const resKey = nodeKey(item);
+                                                    const contentStart = resKey.substr(0, Math.floor(resKey.length / 2));
+                                                    let contentEnd = resKey.substr(-Math.floor(resKey.length / 2));
+                                                    // Fix Latin letters being rendered left to right on ellipsis with direction set to rtl


Updating this comment as // Second part of resource path needs 'direction: rtl;' to make ellipsis in the beginning of the string instead of the end. And because we've direction swapped, we now trip on strong LTR characters, so this extra logic fixes it.

I'm not sure I'd write it like this...

I was thinking more like:

// We want the ellipsis to be in the middle of our text, so we use RTL layout to put it there. // Unfortunately, strong LTR characters get jumbled around, so make sure that the last character isn't strong.

For reference, strong is a technical term:
https://en.wikipedia.org/wiki/Bidirectional_text#Strong_characters

Signed-off-by: ciiay <yicai@redhat.com>

jsoref · 2021-08-18T15:12:47Z

ui/src/app/applications/components/application-sync-panel/application-sync-panel.scss

+        label::after {
+            content: attr(content-end);
+            text-overflow: clip;
+            direction: rtl; // This is for ellipsis in the middle instead of the end of resource path


Suggested change

direction: rtl; // This is for ellipsis in the middle instead of the end of resource path

direction: rtl; // This is to help put the ellipsis in the middle instead of at the end of the resource path

rbreeze · 2021-08-18T16:40:29Z

ui/src/app/applications/components/application-sync-panel/application-sync-panel.tsx

+                                                            <CheckboxField id={resKey} field={`resources[${i}]`} />
+                                                            <Tooltip content={<div style={{wordBreak: 'break-all'}}>{resKey}</div>}>
+                                                                <div className='container'>
+                                                                    <label htmlFor={resKey} style={{display: resKey.length <= 68 ? '' : 'none'}}>


Instead of setting the display property of these elements conditionally, I think it would be simpler to do something like:

isLongLabel = resKey.length > 68; ... <label htmlFor={resKey} content-start={isLongLabel && contentStart} content-end={isLongLabel && contentEnd} />

Also, I'm can't find documentation about the content-start or content-end properties. Could you explain what they do?

They're custom things which are used by the css.

I'm not sure I'm a huge fan of the names, although I don't offhand have a suggestion for better names.

@rbreeze Good point on the inline style. I'm removing it. Please confirm updated PR. Thanks for the review.

ui/src/app/applications/components/application-sync-panel/application-sync-panel.scss

jsoref · 2021-08-18T16:45:25Z

ui/src/app/applications/components/application-sync-panel/application-sync-panel.tsx

+                                                            <CheckboxField id={resKey} field={`resources[${i}]`} />
+                                                            <Tooltip content={<div style={{wordBreak: 'break-all'}}>{resKey}</div>}>
+                                                                <div className='container'>
+                                                                    <label htmlFor={resKey} style={{display: resKey.length <= 68 ? '' : 'none'}}>


They're custom things which are used by the css.

I'm not sure I'm a huge fan of the names, although I don't offhand have a suggestion for better names.

…4233-add-tooltips-for-truncated-check-box-label

Signed-off-by: ciiay <yicai@redhat.com>

rbreeze

Is there a way to avoid the character in the middle getting cut off?

Personally I think it seems like a lot of complexity to add for something as small as moving ellipses. I think it's preferable to keep the ellipses to the right (with pure CSS) and simply add a tooltip.

ciiay · 2021-08-20T14:54:38Z

Is there a way to avoid the character in the middle getting cut off?

Personally I think it seems like a lot of complexity to add for something as small as moving ellipses. I think it's preferable to keep the ellipses to the right (with pure CSS) and simply add a tooltip.

It's either cutting off letters in the middle or 6 dots for the ellipsis. 3 from first part of content, 3 from the second part of the content.

I agree it's adding too much complexity for moving ellipsis. Since we already added tooltip, there's no problem to see the full path of resource.

@jsoref How do you think?

jsoref · 2021-08-20T15:00:06Z

I like the PR as proposed.

Ellipsis indicate clipped content. It doesn't matter much how that segment is clipped, if someone cares, they'll hover.

But ellipsis of the end results in dozens of identical items and no hint as to which should be hovered. That's the bug that this PR addresses.

There might be a css property to get character oriented clipping instead of pixel oriented…

rbreeze · 2021-08-20T16:56:41Z

ui/src/app/applications/components/application-sync-panel/application-sync-panel.tsx

+                                                            <CheckboxField id={resKey} field={`resources[${i}]`} />
+                                                            <Tooltip content={<div style={{wordBreak: 'break-all'}}>{resKey}</div>}>
+                                                                <div className='container'>
+                                                                    {!isLongLabel && <label htmlFor={resKey}>{resKey}</label>}


Can we use a ternary operator here?

{isLongLabel ? <label htmlFor={resKey} content-start={contentStart} content-end={contentEnd} /> : <label htmlFor={resKey}>{resKey}</label>}

Yeah, ternary operator is better. Thanks for the feedback. Changing to ternary operator.

…4233-add-tooltips-for-truncated-check-box-label

Signed-off-by: ciiay <yicai@redhat.com>

rbreeze

LGTM

fix: Truncated synchronize check-boxes should have tooltips argoproj#…

3ce02ec

…4233 Signed-off-by: ciiay <yicai@redhat.com>

jsoref reviewed Aug 11, 2021

View reviewed changes

Merge branch 'master' of https://github.com/argoproj/argo-cd into ui-…

c132a2a

…4233-add-tooltips-for-truncated-check-box-label

jsoref reviewed Aug 17, 2021

View reviewed changes

ciiay commented Aug 17, 2021

View reviewed changes

ciiay added 2 commits August 17, 2021 16:10

fix: updated comments

ba1d9b0

Signed-off-by: ciiay <yicai@redhat.com>

fix: updated comments

a17ae8e

Signed-off-by: ciiay <yicai@redhat.com>

jsoref approved these changes Aug 18, 2021

View reviewed changes

rbreeze requested changes Aug 18, 2021

View reviewed changes

jsoref reviewed Aug 18, 2021

View reviewed changes

ciiay added 3 commits August 18, 2021 13:56

Merge branch 'master' of https://github.com/argoproj/argo-cd into ui-…

64ac123

…4233-add-tooltips-for-truncated-check-box-label

updated for comments

6d5afed

Signed-off-by: ciiay <yicai@redhat.com>

updated for comments

9e346c2

Signed-off-by: ciiay <yicai@redhat.com>

rbreeze requested changes Aug 20, 2021

View reviewed changes

ciiay added 2 commits August 20, 2021 13:28

Merge branch 'master' of https://github.com/argoproj/argo-cd into ui-…

7f889d0

…4233-add-tooltips-for-truncated-check-box-label

Simplified code

5cf5f63

Signed-off-by: ciiay <yicai@redhat.com>

rbreeze approved these changes Aug 20, 2021

View reviewed changes

rbreeze merged commit d12eafa into argoproj:master Aug 20, 2021

	direction: rtl; // This is for ellipsis in the middle instead of the end of resource path
	direction: rtl; // This is to help put the ellipsis in the middle instead of at the end of the resource path

fix: Truncated synchronize check-boxes should have tooltips #4233 #6954

fix: Truncated synchronize check-boxes should have tooltips #4233 #6954

Uh oh!

Conversation

ciiay commented Aug 11, 2021

Uh oh!

codecov bot commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciiay Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciiay Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciiay Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbreeze left a comment

Choose a reason for hiding this comment

Uh oh!

ciiay commented Aug 20, 2021

Uh oh!

jsoref commented Aug 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbreeze left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Aug 11, 2021 •

edited

Loading

ciiay Aug 11, 2021 •

edited

Loading

ciiay Aug 17, 2021 •

edited

Loading

ciiay Aug 17, 2021 •

edited

Loading