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.

Sign up for GitHub

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

Jump to bottom

feat(pci.private-network): Add link to string 'En savoir plus ici' on vlan config step #13304

Merged
merged 6 commits into from
Oct 10, 2024
Merged

feat(pci.private-network): Add link to string 'En savoir plus ici' on vlan config step #13304

merged 6 commits into from
Oct 10, 2024

Conversation

Tsiorifamonjena
Copy link
Contributor

@Tsiorifamonjena Tsiorifamonjena commented Sep 26, 2024 •
edited
Loading

ref: TAPC-735

Question Answer
Branch? develop
Bug fix? no
New feature? no
Breaking change? no
Tickets Fix #TAPC-735
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

@Tsiorifamonjena Tsiorifamonjena marked this pull request as draft September 26, 2024 14:39
@Tsiorifamonjena Tsiorifamonjena marked this pull request as ready for review September 27, 2024 13:50
fredericvilcot
fredericvilcot previously approved these changes Sep 30, 2024
View reviewed changes
SimonChaumet
SimonChaumet previously approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@SimonChaumet SimonChaumet left a comment

Choose a reason for hiding this comment

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

I'm a bit worried about rerendering on the trans component, can you check that there isn't too much rerenders ?

@Tsiorifamonjena
Copy link
Contributor Author

I'm a bit worried about rerendering on the trans component, can you check that there isn't too much rerenders ?

@SimonChaumet that's seem good to me but I'm interested in your point of view. By the way a rework will coming soon for the entire page then I'll make sure we optimize

const

ThibaudCrespin
ThibaudCrespin reviewed Oct 2, 2024
View reviewed changes
packages/manager/apps/pci-private-network/src/pages/new/steps/ConfigurationStep.tsx Outdated
@@ -113,6 +113,10 @@ export default function ConfigurationStep({
GUIDE_LINKS.REGION_AVAILABILITY[ovhSubsidiary] ||
GUIDE_LINKS.REGION_AVAILABILITY.DEFAULT;

// TODO: verify rerender
Copy link
Contributor

Choose a reason for hiding this comment

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

something you forgot or legit to keep this TODO here ?

tibs245
tibs245 reviewed Oct 3, 2024
View reviewed changes
packages/manager/apps/pci-private-network/src/pages/new/steps/ConfigurationStep.tsx Outdated
Comment on lines 396 to 423
<Trans
t={t}
i18nKey="pci_projects_project_network_private_create_vlan_tip"
components={{
link: (
<a
href={VLAN_GUIDE_URL}
target="_blank"
rel="noreferrer"
>
{tCommon('common_find_out_more_here')}
</a>
),
}}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want block this PR but why don't use a simple :

Suggested change
<Trans
t={t}
i18nKey="pci_projects_project_network_private_create_vlan_tip"
components={{
link: (
<a
href={VLAN_GUIDE_URL}
target="_blank"
rel="noreferrer"
>
{tCommon('common_find_out_more_here')}
</a>
),
}}
/>
{t('pci_projects_project_network_private_create_vlan_tip")}
<a
href={VLAN_GUIDE_URL}
target="_blank"
rel="noreferrer"
>
{tCommon('common_find_out_more_here')}
</a>

?

Copy link
Contributor

Choose a reason for hiding this comment

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

And replace <a /> by Link component for have unified behaviour 🙂
packages/manager-react-components/src/components/typography/links/links.component.tsx

https://ovh.github.io/manager/storybook-static/index.html?path=/story/typography-links--external-link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, nice catch thank you

antonymarion
antonymarion previously approved these changes Oct 4, 2024
View reviewed changes
Eric-ciccotti
Eric-ciccotti previously approved these changes Oct 4, 2024
View reviewed changes
SimonChaumet
SimonChaumet previously approved these changes Oct 4, 2024
View reviewed changes
fredericvilcot
fredericvilcot previously approved these changes Oct 4, 2024
View reviewed changes
@github-actions github-actions bot added the has conflicts Has conflicts to resolve before merging label Oct 8, 2024
@Tsiorifamonjena Tsiorifamonjena force-pushed the feat/pci-private-network_addLink_tapc-735 branch from ea32ff9 to 8417058 Compare October 8, 2024 13:50
@github-actions github-actions bot removed the has conflicts Has conflicts to resolve before merging label Oct 8, 2024
@anooparveti anooparveti changed the base branch from develop to release/public-cloud-w42 October 10, 2024 14:18
@github-actions github-actions bot added the has conflicts Has conflicts to resolve before merging label Oct 10, 2024
@Tsiorifamonjena Tsiorifamonjena force-pushed the feat/pci-private-network_addLink_tapc-735 branch from 368dee7 to 02249e0 Compare October 10, 2024 14:38
@github-actions github-actions bot removed the has conflicts Has conflicts to resolve before merging label Oct 10, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@anooparveti anooparveti merged commit 813b00f into release/public-cloud-w42 Oct 10, 2024
11 of 14 checks passed
@anooparveti anooparveti deleted the feat/pci-private-network_addLink_tapc-735 branch October 10, 2024 14:43
@anooparveti anooparveti mentioned this pull request Oct 10, 2024
Merged
10 tasks
sachinrameshn pushed a commit that referenced this pull request Oct 14, 2024
@Tsiorifamonjena @ovh-cds @sachinrameshn
feat(pci-private-network): add link to string 'En savoir plus ici' on…
f9108f8 
… vlan config step (#13304)

ref: TAPC-735

Signed-off-by: tsiorifamonjena <[email protected]>
Co-authored-by: CDS Translator Agent <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredericvilcot fredericvilcot fredericvilcot approved these changes

@SimonChaumet SimonChaumet SimonChaumet approved these changes

@tibs245 tibs245 tibs245 approved these changes

@kqesar kqesar Awaiting requested review from kqesar kqesar was automatically assigned from ovh/su-digital-tools-core-stack

@frenautvh frenautvh Awaiting requested review from frenautvh frenautvh was automatically assigned from ovh/su-digital-tools-core-stack

@sachinrameshn sachinrameshn Awaiting requested review from sachinrameshn sachinrameshn was automatically assigned from ovh/team-manager-dev-angularjs

@mhelhali-soufien mhelhali-soufien Awaiting requested review from mhelhali-soufien mhelhali-soufien was automatically assigned from ovh/team-manager-dev-angularjs

@ThibaudCrespin ThibaudCrespin Awaiting requested review from ThibaudCrespin

@antonymarion antonymarion Awaiting requested review from antonymarion

@Eric-ciccotti Eric-ciccotti Awaiting requested review from Eric-ciccotti

Assignees
No one assigned
Labels
feature New feature quality check: ok translation required universe-public-cloud
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Tsiorifamonjena @tibs245 @SimonChaumet @antonymarion @ThibaudCrespin @Eric-ciccotti @fredericvilcot @anooparveti