-
Notifications
You must be signed in to change notification settings - Fork 28
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
Questionmark tooltip #325
Questionmark tooltip #325
Conversation
✅ Deploy Preview for lodestone-dashboard ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for lodestone-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -15,6 +15,7 @@ import { useGlobalSettings } from 'data/GlobalSettings'; | |||
import { LodestoneContext } from 'data/LodestoneContext'; | |||
import { useCoreInfo } from 'data/SystemInfo'; | |||
import { useDocumentTitle } from 'usehooks-ts'; | |||
import TooltipButton from 'components/Tooltip'; |
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.
where's this being used?
dashboard/src/components/Tooltip.tsx
Outdated
}); | ||
|
||
return ( | ||
<div> |
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.
is this outer div necessary?
dashboard/src/components/Tooltip.tsx
Outdated
import clsx from 'clsx'; | ||
|
||
|
||
const Tooltip = ({ |
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.
might be best to pick a more specific name/make this component more general since we hard coded an "info" icon. Maybe something like "InfoTooltip"?
I think we have a few examples of using Also would be nice if we can add it to more places |
package.json
Outdated
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.
let's remove these accidentally created package.json files
@@ -43,6 +44,8 @@ export default function InputBox({ | |||
isLoading: isLoadingProp = false, | |||
id = '', | |||
showIcons = true, | |||
tooltip = false, | |||
tooltipText, |
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.
might be nice to use just 1 prop for tooltip,
since we want to enable tooltip if and only if we have tooltip text
tooltip: string, | ||
}) => { | ||
return ( | ||
<> |
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.
would be nice to remove the unneeded fragment <> </>
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.
lgtm!
adds the tooltip. (not a questionmark but an i icon to go along with what we already have in the core instance creation menu)
only added it to one place so far though so if we want it anywhere else let me know and i'll add that to the commit