feat(ClauseHeader): added tooltips to icons #318

elit-altum · 2020-03-01T14:12:50Z

Signed-off-by: elit-altum [email protected]

Continues: accordproject/markdown-editor#124

Changes

Modified the ClauseBackground UI according to the proposed changes.
Added tooltips on all the ClauseHeader icons for more information.

Screenshot

Flags

Design modifications and suggestions are welcome!

Related Issues

Pull Request: chore(dep) Upgrade Markdown Transform 0.9.2, Editor 0.9.2, Cicero 0.20.5 #247

elit-altum · 2020-03-01T14:19:11Z

@irmerk @DianaLease Please review!
The failing build is due to a failing snapshot test for the ClauseHeader icons. If you approve the changes I'll update the tests as well.

sachdeva-shrey · 2020-03-01T14:29:51Z

Screenshot

This looks good @elit-altum. I have a few suggestions though

Keep the styling(color, border-radius, background-color) of the icons and ToolbarTooltip consistent with that of Markdown Editor
Increase the width of the editor by around 30%
Increase left and right margin of the text for a cleaner look

elit-altum · 2020-03-01T16:06:30Z

Screenshot

This looks good @elit-altum. I have a few suggestions though

Keep the styling(color, border-radius, background-color) of the icons and ToolbarTooltip consistent with that of Markdown Editor

Increase the width of the editor by around 30%

Increase left and right margin of the text for a cleaner look

Thank you for the review. Actually most of these values are controlled by props passed to the component. Thereby, they will be changed accordingly.
Will surely work on the spacing though.

jolanglinais · 2020-03-01T23:58:16Z

Tagging @Michael-Grover @dselman @jeromesimeon @DianaLease @mttrbrts for feedback

elit-altum · 2020-03-04T14:16:50Z

@irmerk any suggestions on this?

Michael-Grover · 2020-03-04T15:03:46Z

Applications that use the Cicero UI handle the tooltips on clause templates on their own. I am not sure how this will effect that.

Does this remove the green border and border radius around clause templates?

Something appears to be wrong with icon buttons the buttons in the PR. Only a very small area is clickable.

jolanglinais · 2020-03-04T15:09:25Z

Applications that use the Cicero UI handle the tooltips on clause templates on their own. I am not sure how this will effect that.

Not sure what this is referring to, could you elaborate?

Does this remove the green border and border radius around clause templates?

Yes this PR does do that.

Michael-Grover · 2020-03-04T15:17:20Z

Applications that use the Cicero UI handle the tooltips on clause templates on their own. I am not sure how this will effect that.

Not sure what this is referring to, could you elaborate?

Does this remove the green border and border radius around clause templates?

Yes this PR does do that.

In some applications that use Cicero UI, such as Clause, the tooltips appear to be handled on their end. If we add tooltips on the AP side, will that result in icons having two tooltips on hover when viewed in applications that use Cicero UI?

jolanglinais · 2020-03-04T15:23:05Z

I would imagine having this as low level as possible would be best. The icon exists in cicero-ui, so why wouldn't the tooltip explaining the icon live here?

elit-altum · 2020-03-04T15:33:42Z

@Michael-Grover The icon issue might be due to the fact that I reduced the icon sizes to be equivalent in proportions. I'd revert them back on.

elit-altum · 2020-03-04T15:34:42Z

Applications that use the Cicero UI handle the tooltips on clause templates on their own. I am not sure how this will effect that.

Does this remove the green border and border radius around clause templates?

Something appears to be wrong with icon buttons the buttons in the PR. Only a very small area is clickable.

Yes @Michael-Grover the borders and margins are handled. Do you want me to increase the spacing ?

Michael-Grover · 2020-03-04T15:36:17Z

I would imagine having this as low level as possible would be best. The icon exists in cicero-ui, so why wouldn't the tooltip explaining the icon live here?

in that case, @elit-altum please make the styling of the tooltips match this by increasing the padding and adding rounded corners:

Please undo all changes unrelated to the tooltip improvements, such as "Modified the ClauseBackground UI according to the proposed changes." . I believe proposed UI changes should be made in separate github issue so the community can discuss them individually. Some applications that use Cicero UI will be affected by these changes so we also want to give them the opportunity to comment.

elit-altum · 2020-03-05T13:19:18Z

I would imagine having this as low level as possible would be best. The icon exists in cicero-ui, so why wouldn't the tooltip explaining the icon live here?

in that case, @elit-altum please make the styling of the tooltips match this by increasing the padding and adding rounded corners:

Please undo all changes unrelated to the tooltip improvements, such as "Modified the ClauseBackground UI according to the proposed changes." . I believe proposed UI changes should be made in separate github issue so the community can discuss them individually. Some applications that use Cicero UI will be affected by these changes so we also want to give them the opportunity to comment.

All right, no problem!
I'll make a separate PR for that.

elit-altum · 2020-03-05T13:50:02Z

I would imagine having this as low level as possible would be best. The icon exists in cicero-ui, so why wouldn't the tooltip explaining the icon live here?

in that case, @elit-altum please make the styling of the tooltips match this by increasing the padding and adding rounded corners:

Please undo all changes unrelated to the tooltip improvements, such as "Modified the ClauseBackground UI according to the proposed changes." . I believe proposed UI changes should be made in separate github issue so the community can discuss them individually. Some applications that use Cicero UI will be affected by these changes so we also want to give them the opportunity to comment.

@Michael-Grover made the changes to the tooltip and reverted the others
Screenshot

jolanglinais · 2020-03-05T16:07:39Z

@elit-altum There is still some issue with the viewbox or something where the icon isn't consistently clickable on hover.

elit-altum · 2020-03-05T17:09:40Z

@elit-altum There is still some issue with the viewbox or something where the icon isn't consistently clickable on hover.

Yes @irmerk just fixed it!
It was due to use of onMouseEnter() instead of onMouseOver()

Please review!

jolanglinais · 2020-03-05T18:16:49Z

src/components/ClauseComponent.js

+              onMouseOver={() => setHoveringTestIcon(true)}
+              onMouseOut={() => setHoveringTestIcon(false)}


These should be in wrappers instead of the icons themselves. The issue is the mouse going over the explicit lines in the svg are toggling this, instead of the whole area of the icon itself.

@irmerk Is the issue still there ? I think it's fixed in this commit please check once at https://deploy-preview-318--cicero-ui.netlify.com/

I have nested this along with the svg because the hover is on that as well. Therefore, for a coherent behavior that the tooltips are only seen when the cursor is over the icon and the hover effect takes place. I implemented this.
Previously, I used the IconWrapper only

Yes, still there. I'm saying this should look like:

<S.EditWrapper onMouseOver={() => setHoveringEditIcon(true)} onMouseOut={() => setHoveringEditIcon(false)} {...iconWrapperProps} > <S.ClauseIcon {...editIconProps}>

for all the icons.

@irmerk I have made these changes. Please review!

jolanglinais · 2020-03-05T19:15:50Z

Hmm the behavior is still there. Unsure what it is, will need to look into it more and I can take a look later today or tomorrow.

jolanglinais · 2020-03-06T14:55:04Z

I'm going to take a look locally...

elit-altum · 2020-03-06T15:01:31Z

Now I'm getting a slightly different issue:

Okay this could be due to browsers parsing the JS differently. This works fine with Firefox. But I am having similar issue on Chrome. Let me fix it.

elit-altum · 2020-03-06T15:18:43Z

@irmerk I thinks it's fixed. Works fine on my Firefox and Chrome. Please check once!

jolanglinais · 2020-03-06T15:47:35Z

Yes, this is better!

I think the gradient color on the icon wrappers needs to be updated with the new clause background color, and also the wrapper is overlapping the tooltip caret, so maybe make the tooltip have a higher z-index?

elit-altum · 2020-03-06T15:49:33Z

Yes, this is better!

I think the gradient color on the icon wrappers needs to be updated with the new clause background color, and also the wrapper is overlapping the tooltip caret, so maybe make the tooltip have a higher z-index?

Sure @irmerk I'll get this done!

jolanglinais · 2020-03-06T15:51:06Z

src/components/ClauseComponent.js

+            onMouseEnter={() => setHoveringTestIcon(true)}
+            onMouseLeave={() => setHoveringTestIcon(false)}
+            onClick={testIconProps.onClick}


I think onClick here should be () => clauseProps.CLAUSE_TEST_FUNCTION(props), and then that function should be removed from testIconProps

Okay will do!

elit-altum · 2020-03-06T16:45:19Z

@irmerk I have made the required changes. If this is okay I'll proceed to squash my commits (if required)
Please review!

jolanglinais · 2020-03-06T17:07:11Z

Sorry just one last thing: z-index should be higher number. Lower numbers means lower priority, so it's still being covered up by the icon wrapper.

Yeah I think squashing some of the commits together could be good with interactive rebase. If it makes sense to squash all of them then go ahead.

elit-altum · 2020-03-06T17:28:19Z

Sorry just one last thing: z-index should be higher number. Lower numbers means lower priority, so it's still being covered up by the icon wrapper.

Yeah I think squashing some of the commits together could be good with interactive rebase. If it makes sense to squash all of them then go ahead.

@irmerk I had made those changes already.

Here, I have reduced the top and changed background-color of the IconWrapper to show that the z-index is not the problem.

The issue was actually due to the border adding width of its own which is now fixed!

Signed-off-by: elit-altum <[email protected]>

elit-altum · 2020-03-06T17:42:47Z

@irmerk Squashed all commits. Please review!

jolanglinais

Good catch on the z-index / border-width / etc.

This looks great! I think just update the snapshots to pass the tests with npm test -- -u

elit-altum · 2020-03-06T18:05:44Z

npm test -- -u

Running this gives me this error

Could you please help me ?

jolanglinais · 2020-03-06T18:08:09Z

Hmm are you able to run npm test?

elit-altum · 2020-03-06T18:10:17Z

Hmm are you able to run npm test?

No I cannot. It gives a similar error

jolanglinais · 2020-03-06T18:16:02Z

I've cloned your branch and it works... Are you not running npm install first?

elit-altum · 2020-03-06T18:21:52Z

I've cloned your branch and it works... Are you not running npm install first?

That's weird. I ran npm install again. Still doesn't work.

DianaLease

Looks great once the snapshot is updated!

jolanglinais · 2020-03-06T18:32:36Z

Are you on our slack? Might be faster to communicate on this one. Need to debug this because this shouldn't be an issue. Just running jest doesn't even work?

Signed-off-by: irmerk <[email protected]>

Michael-Grover · 2020-03-09T15:44:26Z

I noticed a bug in this feature. I created an issue since this PR is already merged. #330

jolanglinais assigned elit-altum Mar 1, 2020

jolanglinais requested review from jolanglinais, DianaLease, dselman, jeromesimeon, Michael-Grover and mttrbrts March 4, 2020 14:51

jolanglinais added Type: Discussion 🗣 Type: Question ❓ Further information is requested labels Mar 4, 2020

elit-altum mentioned this pull request Mar 5, 2020

Make Clause Template background color lighter #322

Closed

jolanglinais reviewed Mar 5, 2020

View reviewed changes

elit-altum mentioned this pull request Mar 5, 2020

feat(ClauseComponent): UI enhancements to ClauseComponent #324

Closed

elit-altum changed the title ~~feat(ClauseComponent): UI/UX enhancments to ClauseComponent~~ feat(ClauseHeader): added tooltips to icons Mar 6, 2020

elit-altum force-pushed the ClausePlugin-modify branch 3 times, most recently from 39ec021 to 513cf7a Compare March 6, 2020 15:16

jolanglinais reviewed Mar 6, 2020

View reviewed changes

elit-altum force-pushed the ClausePlugin-modify branch from 628ad35 to 56de8d3 Compare March 6, 2020 16:44

feat(ClauseComponent): added tooltips to action icons

f175f7d

Signed-off-by: elit-altum <[email protected]>

elit-altum force-pushed the ClausePlugin-modify branch from 44cedd1 to f175f7d Compare March 6, 2020 17:41

jolanglinais previously approved these changes Mar 6, 2020

View reviewed changes

DianaLease previously approved these changes Mar 6, 2020

View reviewed changes

chore(test): update snapshot

e7992b5

Signed-off-by: irmerk <[email protected]>

jolanglinais dismissed stale reviews from DianaLease and themself via e7992b5 March 6, 2020 19:01

jolanglinais approved these changes Mar 6, 2020

View reviewed changes

jolanglinais merged commit 26eecd0 into accordproject:master Mar 6, 2020

		onMouseOver={() => setHoveringTestIcon(true)}
		onMouseOut={() => setHoveringTestIcon(false)}

feat(ClauseHeader): added tooltips to icons #318

feat(ClauseHeader): added tooltips to icons #318

Conversation

elit-altum commented Mar 1, 2020

Changes

Screenshot

Flags

Related Issues

elit-altum commented Mar 1, 2020

sachdeva-shrey commented Mar 1, 2020 • edited Loading

Screenshot

elit-altum commented Mar 1, 2020

Screenshot

jolanglinais commented Mar 1, 2020

elit-altum commented Mar 4, 2020

Michael-Grover commented Mar 4, 2020

jolanglinais commented Mar 4, 2020

Michael-Grover commented Mar 4, 2020

jolanglinais commented Mar 4, 2020

elit-altum commented Mar 4, 2020

elit-altum commented Mar 4, 2020

Michael-Grover commented Mar 4, 2020 • edited Loading

elit-altum commented Mar 5, 2020

elit-altum commented Mar 5, 2020

jolanglinais commented Mar 5, 2020

elit-altum commented Mar 5, 2020 • edited Loading

jolanglinais Mar 5, 2020

Choose a reason for hiding this comment

elit-altum Mar 5, 2020

Choose a reason for hiding this comment

jolanglinais Mar 5, 2020

Choose a reason for hiding this comment

elit-altum Mar 5, 2020

Choose a reason for hiding this comment

jolanglinais commented Mar 5, 2020

jolanglinais commented Mar 6, 2020

elit-altum commented Mar 6, 2020 • edited Loading

elit-altum commented Mar 6, 2020

jolanglinais commented Mar 6, 2020

elit-altum commented Mar 6, 2020

jolanglinais Mar 6, 2020

Choose a reason for hiding this comment

elit-altum Mar 6, 2020

Choose a reason for hiding this comment

elit-altum commented Mar 6, 2020

jolanglinais commented Mar 6, 2020

elit-altum commented Mar 6, 2020 • edited Loading

elit-altum commented Mar 6, 2020

jolanglinais left a comment

Choose a reason for hiding this comment

elit-altum commented Mar 6, 2020

jolanglinais commented Mar 6, 2020

elit-altum commented Mar 6, 2020

jolanglinais commented Mar 6, 2020

elit-altum commented Mar 6, 2020

DianaLease left a comment

Choose a reason for hiding this comment

jolanglinais commented Mar 6, 2020

Michael-Grover commented Mar 9, 2020

sachdeva-shrey commented Mar 1, 2020 •

edited

Loading

Michael-Grover commented Mar 4, 2020 •

edited

Loading

elit-altum commented Mar 5, 2020 •

edited

Loading

elit-altum commented Mar 6, 2020 •

edited

Loading

elit-altum commented Mar 6, 2020 •

edited

Loading