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

Cnft1 3308 UI add link to nbs 6 search #1959

Merged
merged 25 commits into from
Oct 25, 2024

Conversation

benlam-ignw
Copy link
Collaborator

@benlam-ignw benlam-ignw commented Oct 23, 2024

Description

A separate story will be created to add more detailed tests in apps/modernization-ui/src/design-system/extendedTooltip/ExtendedTooltip.spec.tsx.

Patient search header vs the existing search navigation component conditional rendering is based off of features?.search?.events?.enabled

UPDATED:
Screenshot 2024-10-24 at 11 52 08 AM

OLD:

Screenshot 2024-10-23 at 10 12 19 AM

Tickets

https://cdc-nbs.atlassian.net/browse/CNFT1-3308

Checklist before requesting a review

  • PR focuses on a single story
  • Code has been fully tested to meet acceptance criteria
  • PR is reasonably small and reviewable (Generally less than 10 files and 500 changed lines)
  • All new functions/classes/components reasonably small
  • Functions/classes/components focused on one responsibility
  • Code easy to understand and modify (clarity over concise/clever)
  • PRs containing TypeScript follow the Do's and Don'ts
  • PR does not contain hardcoded values (Uses constants)
  • All code is covered by unit or feature tests

children: ReactNode;
};

const ExtendedTooltip = ({ labelTitle, labelText, position, children }: Props) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: Could you rename this to RichTooltip? That is what the design team is referring to this component as.

question: Isn'tlabelText the same as children here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Sure thing!
  2. Label title and label text are used in the label that gets shown/hidden in the USWDS Tooltip component, while the children like the info outlined icon here are always shown

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that makes sense

@mpeels
Copy link
Collaborator

mpeels commented Oct 23, 2024

Issue: the spacing seems a bit off for the text / icon from Figma
Figma:
image

Copy link
Collaborator

@adamloup-enquizit adamloup-enquizit left a comment

Choose a reason for hiding this comment

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

The placement of the rich tool tip looks a little off, the designs have place under the icon with the left side of the tooltip aligned with it vertically. I don't think that little arrow should be their either.

@benlam-ignw
Copy link
Collaborator Author

Closing for now while I rewrite the RichTooltip component

@benlam-ignw
Copy link
Collaborator Author

I've created custom tooltip logic in the Rich Tooltip component, please check it out and let me know if you have any feedback :)

@benlam-ignw benlam-ignw reopened this Oct 24, 2024
@benlam-ignw benlam-ignw merged commit ec9a423 into main Oct 25, 2024
1 check passed
@benlam-ignw benlam-ignw deleted the CNFT1-3308-UI-Add-link-to-NBS-6-Search branch October 25, 2024 16:04
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.

4 participants