search

accordproject / cicero-ui

A library of React components for Accord Project templates
Apache License 2.0
29 stars 45 forks source link

feat(ClauseHeader): added tooltips to icons #318

Closed elit-altum closed 4 years ago

elit-altum commented 4 years ago

Signed-off-by: elit-altum manan.sharma311@gmail.com

Continues: https://github.com/accordproject/markdown-editor/issues/124

### Changes - Modified the ClauseBackground UI according to the proposed changes. - Added tooltips on all the ClauseHeader icons for more information. ### Screenshot ![Tooltip-fixing](https://user-images.githubusercontent.com/41413622/75627272-be3c6b80-5bf4-11ea-9bb5-bd5a70c7976d.gif) ### Flags - Design modifications and suggestions are welcome! ### Related Issues - Pull Request: #247
elit-altum commented 4 years ago

@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 commented 4 years ago

Screenshot

Tooltip-fixing

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

elit-altum commented 4 years ago

Screenshot

Tooltip-fixing

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 commented 4 years ago

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

elit-altum commented 4 years ago

@irmerk any suggestions on this?

Michael-Grover commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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? image

jolanglinais commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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:

image

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 commented 4 years ago

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:

image

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 commented 4 years ago

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:

image

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 Screenshot (1)

jolanglinais commented 4 years ago

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

elit-altum commented 4 years ago

@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 commented 4 years ago

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.

elit-altum commented 4 years ago

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.

@irmerk Please take a look at these. The original hover effect on https://cicero-ui.netlify.com/ and compare it with now.

(moving cursor over the outlines)

Original Tooltip-fixing-original

With Tooltip Tooltip-fixing-after

I think this behavior was present originally itself.

The icons not being consistently clickable could be because the ::hover tag is set on the ClauseIcon and not the IconWrapper (leading to similar behavior as https://github.com/accordproject/cicero-ui/pull/318#discussion_r388473717)
I don't think it's because of the tooltips. I can make a separate issue and PR to fix this i.e use the onHover state to pass props to the styled.div of the icons to change color. Thank you!

jolanglinais commented 4 years ago

Weird that I'm getting different behavior:

Mar-06-2020 09-20-34

jolanglinais commented 4 years ago

I think this behavior was present originally itself.

The icons not being consistently clickable could be because the ::hover tag is set on the ClauseIcon and not the IconWrapper (leading to similar behavior as #318 (comment)) I don't think it's because of the tooltips. I can make a separate issue and PR to fix this i.e use the onHover state to pass props to the styled.div of the icons to change color.

Oh I think I get it. No I think we should fix that in this PR as well. In the current demo this isn't as much of an issue but the hover should be on the wrapper as you say.

elit-altum commented 4 years ago

Weird that I'm getting different behavior:

Mar-06-2020 09-20-34

Did this come up only after the tooltips or is it happening in the original demo as well ? That is strange.

elit-altum commented 4 years ago

I think this behavior was present originally itself. The icons not being consistently clickable could be because the ::hover tag is set on the ClauseIcon and not the IconWrapper (leading to similar behavior as #318 (comment)) I don't think it's because of the tooltips. I can make a separate issue and PR to fix this i.e use the onHover state to pass props to the styled.div of the icons to change color.

Oh I think I get it. No I think we should fix that in this PR as well. In the current demo this isn't as much of an issue but the hover should be on the wrapper as you say.

Okay I will fix this. Should I open a separate issue by the way ?

jolanglinais commented 4 years ago

I think this can just stay in this PR to not get confusing. And yes this isn't happening in the current demo.

elit-altum commented 4 years ago

@irmerk Please review it now!

jolanglinais commented 4 years ago

Now I'm getting a slightly different issue:

Mar-06-2020 09-53-33

jolanglinais commented 4 years ago

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

elit-altum commented 4 years ago

Now I'm getting a slightly different issue:

Mar-06-2020 09-53-33

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 commented 4 years ago

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

jolanglinais commented 4 years ago

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?

Screen Shot 2020-03-06 at 10 42 48 AM
elit-altum commented 4 years ago

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?

Screen Shot 2020-03-06 at 10 42 48 AM

Sure @irmerk I'll get this done!

elit-altum commented 4 years ago

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

jolanglinais commented 4 years ago

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 commented 4 years ago

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. Tooltip-z-index

The issue was actually due to the border adding width of its own which is now fixed! Tooltip-z-index-fix

elit-altum commented 4 years ago

@irmerk Squashed all commits. Please review!

elit-altum commented 4 years ago

npm test -- -u

Running this gives me this error image

Could you please help me ?

jolanglinais commented 4 years ago

Hmm are you able to run npm test?

elit-altum commented 4 years ago

Hmm are you able to run npm test?

No I cannot. It gives a similar error image

jolanglinais commented 4 years ago

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

elit-altum commented 4 years ago

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.

jolanglinais commented 4 years ago

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?

Michael-Grover commented 4 years ago

I noticed a bug in this feature. I created an issue since this PR is already merged. https://github.com/accordproject/cicero-ui/issues/330