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

feat: Added external link icon #4757

Closed
wants to merge 12 commits into from
Closed

feat: Added external link icon #4757

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e2f5522
added external link icon
lavr001 Nov 16, 2023
085c519
removed external source for the link icon, small css updates
lavr001 Nov 22, 2023
c6d775b
updating link test
lavr001 Nov 22, 2023
0cbfb8e
Merge branch 'main' into add-external-link-icon
lavr001 Nov 22, 2023
e9f156e
small fixes based on comments
lavr001 Nov 27, 2023
318e9a6
Merge branch 'add-external-link-icon' of https://github.com/aws-ampli…
lavr001 Nov 27, 2023
0aee71f
one line for the same package import
lavr001 Dec 21, 2023
4e4ff7a
Merge branch 'main' of https://github.com/aws-amplify/amplify-ui into…
lavr001 Jan 9, 2024
9cf0869
added hideIcon and linkIconPosition props
lavr001 Jan 9, 2024
f57b263
Merge branch 'main' into add-external-link-icon
lavr001 Jan 9, 2024
271658b
Merge branch 'main' into add-external-link-icon
lavr001 Jan 10, 2024
5309237
Merge branch 'main' into add-external-link-icon
lavr001 Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions packages/react/src/primitives/Icon/icons/IconBoxArrowUpRight.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import * as React from 'react';
import { classNames } from '@aws-amplify/ui';
import { ComponentClassName } from '@aws-amplify/ui';
Copy link
Member

Choose a reason for hiding this comment

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

This should only be a single import line

Copy link
Member

Choose a reason for hiding this comment

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

Unresolved, there are two import lines for the same package still. In the future, please refrain from resolving a conversation until the feedback has been addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like there are two import lines for the same package across multiple primitives, I'll make sure to make it one line when done with other updates on this PR.

import { View } from '../../View';
import { InternalIcon } from './types';
/**
* @internal For internal Amplify UI use only. May be removed in a future release.
*/
export const IconBoxArrowUpRight: InternalIcon = (props) => {
const { className, ...rest } = props;
return (
<View
as="span"
width="1em"
height="1em"
className={classNames(ComponentClassName.Icon, className)}
{...rest}
>
<svg
width="24"
height="24"
xmlns="http://www.w3.org/2000/svg"
fill="currentColor"
viewBox="0 0 24 24"
>
<path
fill="evenodd"
d="M8.636 3.5a.5.5 0 0 0-.5-.5H1.5A1.5 1.5 0 0 0 0 4.5v10A1.5 1.5 0 0 0 1.5 16h10a1.5 1.5 0 0 0 1.5-1.5V7.864a.5.5 0 0 0-1 0V14.5a.5.5 0 0 1-.5.5h-10a.5.5 0 0 1-.5-.5v-10a.5.5 0 0 1 .5-.5h6.636a.5.5 0 0 0 .5-.5z"
/>
<path
fill="evenodd"
d="M16 .5a.5.5 0 0 0-.5-.5h-5a.5.5 0 0 0 0 1h3.793L6.146 9.146a.5.5 0 1 0 .708.708L15 1.707V5.5a.5.5 0 0 0 1 0v-5z"
/>
</svg>
</View>
);
};
6 changes: 3 additions & 3 deletions packages/react/src/primitives/Link/Link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import * as React from 'react';
import { classNames } from '@aws-amplify/ui';

import { ComponentClassName } from '@aws-amplify/ui';
import { ComponentClassName, classNames } from '@aws-amplify/ui';
import {
BaseLinkProps,
LinkProps,
Expand All @@ -10,6 +8,7 @@ import {
} from '../types';
import { View } from '../View';
import { primitiveWithForwardRef } from '../utils/primitiveWithForwardRef';
import { IconBoxArrowUpRight } from '../Icon/icons/IconBoxArrowUpRight';

const LinkPrimitive: Primitive<LinkProps, 'a'> = (
{ as = 'a', children, className, isExternal, ...rest },
Expand All @@ -25,6 +24,7 @@ const LinkPrimitive: Primitive<LinkProps, 'a'> = (
{...rest}
>
{children}
{isExternal && <IconBoxArrowUpRight />}
Copy link
Member

Choose a reason for hiding this comment

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

Few things here:

  • Prefer ternary returning null if isExternal is falsy to prevent this expression from evaluating to false
  • How do i hide the icon?
  • How do i position the icon to the left of the children?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Updated to use a ternary operator
  2. You can hide the icon when isExternal prop is set to false (default)
  3. It's important to start with a word that indicates the action that results if customers click the link. Putting an icon at the beginning of the link disrupts the readability of the link quite massively, therefore it is a common convention to put icons after the actual link text

Copy link
Member

Choose a reason for hiding this comment

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

You can hide the icon when isExternal prop is set to false (default)

Sure, but what if the link is external and i don't want the icon?

It's important to start with a word that indicates the action that results if customers click the link. Putting an icon at the beginning of the link disrupts the readability of the link quite massively, therefore it is a common convention to put icons after the actual link text

  1. What abt RTL languages?
  2. This response is an opinion on the usage of the Link component but does not address the actual question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call on RTL languages. I'll work on adding a position prop, so devs can control the icon position

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no need to create a position prop since the direction='rtl' can be set in the ThemeProvider and it will automatically applied to the Link component
Screenshot 2023-12-04 at 4 00 24 PM

Copy link
Member

Choose a reason for hiding this comment

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

@lavr001 The above question is still unanswered:

You can hide the icon when isExternal prop is set to false (default)

Sure, but what if the link is external and i don't want the icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@calebpollman Are you suggesting having a new prop called hasIcon that would be visible only when the link is external, so users can control if they want the icon or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@calebpollman Added new prop hideIcon, so now if the Link is external you have an option to show/hide the icon. Also added a new prop linkIconPosition, so you can position the icon left or right to the Link
Screenshot 2024-01-09 at 2 54 22 PM

</View>
);
};
Expand Down
9 changes: 9 additions & 0 deletions packages/react/src/primitives/Link/__tests__/Link.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,15 @@ describe('Link:', () => {
expect(link).toHaveAttribute('rel', 'noopener noreferrer');
});

it('should render svg icon when link is external', async () => {
render(<Link isExternal>{linkText}</Link>);

const link = await screen.findByText(linkText);
const span = link.children[0];
expect(span.children.length).toBe(1);
expect(span.children[0].tagName).toBe('svg');
});

it('can render the Link tag as other components', async () => {
render(<Link as={Text}>{linkText}</Link>);

Expand Down
16 changes: 16 additions & 0 deletions packages/ui/src/icons/box_arrow_up_right.svg
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did this icon come from and why is it not centered like the other icons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we don't typically use icons from the external libraries, I just looked online for a SVG path that forms the icon. I've added align-items: center to make the icon vertically aligned, but I can take another look to see what else could be done to make it more vertically centered

Copy link
Contributor

Choose a reason for hiding this comment

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

We are using a specific icon set from material, so we should be consistent and use the same one for this too. @dbanksdesign Do you remember which version we were using?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked SVG icons from material and they didn't really have a specific external link icon, so I grabbed another SVG

Copy link
Member

Choose a reason for hiding this comment

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

Where is this SVG taken from and what was the reasoning used in choosing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simply googled svg path for external icon, since we don't usually pull in icons from an external source. Not sure what do you mean by reasoning. Are you suggesting some other approaches here?

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions packages/ui/src/theme/css/component/link.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
// all:unset removes this, so we have to add it back.
cursor: pointer;

display: inline-flex;
align-items: center;
gap: var(--amplify-space-xxxs);

&:visited {
color: var(--amplify-components-link-visited-color);
text-decoration: var(--amplify-components-link-visited-text-decoration);
Expand Down
Loading