Skip to content
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

feat(app): add module icon tooltips to robot card #10103

Merged
merged 5 commits into from
Apr 29, 2022

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Apr 27, 2022

closes #8672

Overview

This PR adds tooltips to the module icons on the RobotCard, the tooltips describe what module each icon represents.

Changelog

  • extends optional props in moduleIcon and adds props to moduleIcon in robotCard

Review requests

  • Because toolTip is in the app folder since we want a tooltip for the app and a tooltip in components, I added a few tooltip props to moduleIcon is that okay?
  • Does it match designs?

Risk assessment

low

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #10103 (b141f58) into edge (cd25aea) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #10103   +/-   ##
=======================================
  Coverage   74.64%   74.64%           
=======================================
  Files        2091     2092    +1     
  Lines       55177    55180    +3     
  Branches     5553     5553           
=======================================
+ Hits        41187    41190    +3     
  Misses      12866    12866           
  Partials     1124     1124           
Flag Coverage Δ
app 71.04% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/src/atoms/Tooltip/index.tsx 100.00% <ø> (ø)
app/src/organisms/Devices/RobotCard.tsx 50.00% <ø> (ø)
app/src/molecules/ModuleIcon/index.tsx 100.00% <100.00%> (ø)

Comment on lines 23 to 24
moduleIconTooltip?: React.ReactNode
iconTargetProps?: UseHoverTooltipTargetProps
Copy link
Collaborator Author

@jerader jerader Apr 27, 2022

Choose a reason for hiding this comment

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

had to add these optional tooltip props since Tooltip lives in the App folder and this usage of moduleIcon is the only time we need a tooltip

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it true that all ModuleIcon instances should have these tooltips? if that is the case, maybe this component (which I believe is only used in the app right now), should actually be pulled out of components and into app. Then it would not need these new props, as we could use the hover tooltip hook inside of the component itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ModuleIcon is also being used in PD and the only time a tooltip is needed is the MouldeIcon used in the robot card

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe it would be cleaner if the app had its own ModuleIcon molecule, that imports this base component from the components library, and adds in the tooltip. That way this shared component and the app specific tooltip version are still just a pure functions of moduleType.

@jerader jerader marked this pull request as ready for review April 27, 2022 16:38
@jerader jerader requested a review from a team as a code owner April 27, 2022 16:38
@jerader jerader requested review from X-sam, sakibh, shlokamin and b-cooper and removed request for a team and X-sam April 27, 2022 16:38
@jerader jerader force-pushed the app_module-icon-tooltips-robot-card branch from e5f86cd to fb71870 Compare April 29, 2022 18:16
Copy link
Contributor

@sakibh sakibh left a comment

Choose a reason for hiding this comment

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

LGTM! The tooltips look awesome ✅

@jerader jerader merged commit 20472f4 into edge Apr 29, 2022
@jerader jerader deleted the app_module-icon-tooltips-robot-card branch April 29, 2022 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Devices Landing: Module Tooltips
3 participants