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(ext-links): Adding external icon to links based on a parameter - FRONT-3601 #2384

Merged
merged 7 commits into from
Apr 26, 2022

Conversation

planctus
Copy link
Contributor

@planctus planctus commented Apr 5, 2022

The approach used is to give priority to the external icon:
it means that if the link is set to be external it will take precedence on any other "icon" that might have been passed to the component.
This can be tested by setting an icon through the controls, if the link is external nothing should change.

@github-actions
Copy link

github-actions bot commented Apr 5, 2022

@@ -12,6 +12,8 @@ module.exports = {
label: 'Better regulation',
path: exampleLink,
level: 1,
external: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

for the demo to COMM we can have the external link by default, but I guess that on the end we should have the normal link here (and maybe a control to have it external)

@@ -16,6 +16,8 @@ module.exports = {
type: 'standalone',
label: 'Title',
path: exampleLink,
icon_path: 'icons.svg',
external: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

same remark for the default external link

{ label: 'Item 4.13', path: exampleLink },
{ label: 'Item 4.14', path: exampleLink },
],
external: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

we should update the translated data file too

@planctus
Copy link
Contributor Author

planctus commented Apr 13, 2022

Thanks Romain for reviewing this, the examples were meant to show where the "functionality" was added to give a chance both to comm and platform to see what the result would have been. I agree that if this gets "validated" by our stakeholders we can simply remove the icon in some of the stories where we are currently showing it, i would also keep the control to activate it only for the link stories.

@planctus
Copy link
Contributor Author

It still not entirely clear how are we going to release this, the pr has been kept against v3, the examples have been removed, the link is the main "entry" point to test the "external icon" using the associated control

@planctus
Copy link
Contributor Author

The external icon has been added also to the tag component, only when the tag is of type "link"

@planctus planctus merged commit fbb7893 into v3 Apr 26, 2022
@planctus planctus deleted the feat-external-icon-front-3601 branch April 26, 2022 10:36
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.

2 participants