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(Tooltip): adjust style to fit doc #486

Merged
merged 1 commit into from
Aug 8, 2023
Merged

feat(Tooltip): adjust style to fit doc #486

merged 1 commit into from
Aug 8, 2023

Conversation

JsGarneau
Copy link
Contributor

Bug fix checklist

  • Units tests have been adjusted to account for bug.
  • The fix has been tested in multiple Storybook stories.
  • All GitHub checks are successful.

New component checklist

  • The new component and its tests are in the same component folder.
  • The component is unit tested and/or snapshot tested.
  • All of the relevant Storybook stories have been added to the storybook package.
  • There are no linting errors or warnings in the modified/new code.
  • All GitHub checks are successful.

@JsGarneau JsGarneau requested a review from a team as a code owner March 21, 2023 15:12
Copy link
Contributor

@meriouma meriouma left a comment

Choose a reason for hiding this comment

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

On dirait que le radius est pas tout à fait bon visuellement, même si c'est 2px comme dans Figma. Aussi on dirait qu'il y a un peu trop d'espace autour du texte, je pense que le tooltip est un peu plus haut que dans les maquettes. Ça prendrait le review de @LarryMatte je pense.

Copy link
Contributor

@LarryMatte LarryMatte left a comment

Choose a reason for hiding this comment

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

Quelques petites modifications.
@meriouma a raison pour le border de 2px qui fit avec ce qui est dans les specs de figma mais qui ne fit pas vraiment visuellement...
Je crois que je laisserais comme c'est là.
Je vais probablement refaire une passe de CSS de tous les composants quand on aura le feu vert et qu'on saura ce qui se passe avec tout ça.
D'ici là, disons que c'est bien correct.

packages/react/src/components/tooltip/tooltip.tsx Outdated Show resolved Hide resolved
packages/react/src/components/tooltip/tooltip.tsx Outdated Show resolved Hide resolved
packages/react/src/components/tooltip/tooltip.tsx Outdated Show resolved Hide resolved
packages/react/src/components/tooltip/tooltip.tsx Outdated Show resolved Hide resolved
@JsGarneau JsGarneau force-pushed the dev/DS-655 branch 2 times, most recently from 37b3a79 to 872cd15 Compare April 28, 2023 14:56
@@ -270,14 +270,14 @@ label + .c3 {
<input
class="c5 datePickerInput"
data-testid="text-input"
placeholder="YYYY-MM-DD"
placeholder="MM/DD/YYYY"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ta version de node doit pas être contrôlée par asdf. C'est un problème avec les versions plus récentes de node. Ça devrait être corrigé dans la prochaine, mais en attendant tu devrais t'assurer d'utiliser la version de node qu'il y a dans le .tool-versions pour le DS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ca ma fait investiguer les configs de mon asdf. On dirait que ca avait jamais été pluggué correctement 😅

C't'un peu gênant

@JsGarneau JsGarneau force-pushed the dev/DS-655 branch 2 times, most recently from 1272536 to 9e4f18a Compare July 17, 2023 16:18
@@ -188,7 +187,7 @@ export const Tooltip: FunctionComponent<PropsWithChildren<TooltipProps>> = ({
const Theme = useTheme();
const tooltipId = useMemo(uuid, []);
const tooltipTriggerId = useMemo(() => `tooltip-trigger-${tooltipId}`, [tooltipId]);
const [isVisible, setIsVisible] = useState(defaultOpen);
const [isVisible, setIsVisible] = useState(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Vestige de debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor

@meriouma meriouma Jul 25, 2023

Choose a reason for hiding this comment

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

Je pense que t'as pas updaté le snapshot 😅

@JsGarneau JsGarneau force-pushed the dev/DS-655 branch 2 times, most recently from 73110d1 to facaa18 Compare July 26, 2023 14:11
@JsGarneau JsGarneau merged commit 606d0c3 into master Aug 8, 2023
20 checks passed
@JsGarneau JsGarneau deleted the dev/DS-655 branch August 8, 2023 17:14
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.

None yet

3 participants