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(hycu): add link to manage contacts #14263

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pauldkn
Copy link
Contributor

@pauldkn pauldkn commented Nov 25, 2024

ref: MANAGER-15782

Question Answer
Branch? master
Bug fix? no
New feature? yes
Breaking change? no
Tickets feat/MANAGER-15782
License BSD 3-Clause
  • Try to keep pull requests small so they can be easily reviewed.
  • Commits are signed-off
  • Only FR translations have been updated
  • Branch is up-to-date with target branch
  • Lint has passed locally
  • Standalone app was ran and tested locally
  • Ticket reference is mentioned in linked commits (internal only)
  • Breaking change is mentioned in relevant commits

Description

Related

  ref: MANAGER-15782

Signed-off-by: Paul Dickerson <[email protected]>
Copy link

sonarcloud bot commented Nov 25, 2024

Comment on lines +123 to +132
{isLoadingContactUrl ? (
<OsdsSkeleton />
) : (
<Links
href={(contactUrl as string) ?? '#'}
type={LinkType.next}
label={t('hycu_dashboard_field_label_manage_contacts')}
className="mt-4"
/>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just little suggestion with a question :

When I implemented this I use disabled because the content not change and the load is very fast (Is not a web request just a local async function)

So I find this more readable, more maintainable and have better UX / CLS

What do you think ?

But is good implementation too

Copy link
Contributor

@chipp972 chipp972 Nov 26, 2024

Choose a reason for hiding this comment

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

+1 good idea

Comment on lines +111 to +113
Array.from({ length: 3 }).map((_, index) => (
<OsdsSkeleton key={index} />
))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Array.from({ length: 3 }).map((_, index) => (
<OsdsSkeleton key={index} />
))
<>
<OsdsSkeleton />
<OsdsSkeleton />
<OsdsSkeleton />
</>
)

No need to re-calculate a list of static components on each render I think

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the loop is better to maintain the code and avoid repetition. Maybe another opinion to known what method to choose ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants