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

Fix link icon alignment #7996

Merged
merged 14 commits into from
Dec 15, 2023
Merged

Fix link icon alignment #7996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c134ba8
Consolidate all links with icons to use a `iconX` prop.
tofumatt Dec 15, 2023
c425176
Restore ellipsis to Key Metrics links.
tofumatt Dec 15, 2023
c715221
Use number for icons.
tofumatt Dec 15, 2023
1c70e2f
Merge branch 'main' into icon-followup-revert
tofumatt Dec 15, 2023
9f84bcb
Update VRTs and link prop name.
tofumatt Dec 15, 2023
093d70c
Fix visually hidden contents being without whitespace.
tofumatt Dec 15, 2023
d1b1132
Fix button icon alignment.
tofumatt Dec 15, 2023
b4b5b24
Fix all traffic widget labels.
tofumatt Dec 15, 2023
faab5e2
Update VRTs.
tofumatt Dec 15, 2023
021ae9d
Fix all traffic widget label alignment.
tofumatt Dec 15, 2023
d42af85
Update VRTs.
tofumatt Dec 15, 2023
c77f617
Correct alignment in km cta actions.
aaemnnosttv Dec 15, 2023
9a03606
Approve visual changes.
aaemnnosttv Dec 15, 2023
03fd24e
Update snapshot tests.
aaemnnosttv Dec 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion assets/js/components/KeyMetrics/ChangeMetricsLink.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ export default function ChangeMetricsLink() {
linkButton
className="googlesitekit-km-change-metrics-cta"
onClick={ openMetricsSelectionPanel }
leadingIcon={ <PencilIcon width={ 22 } height={ 22 } /> }
>
<PencilIcon width={ 22 } height={ 22 } />
{ __( 'Change Metrics', 'google-site-kit' ) }
</Link>
<SetupCompletedSurveyTrigger />
Expand Down
47 changes: 33 additions & 14 deletions assets/js/components/Link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ const Link = forwardRef( ( props, ref ) => {
standalone = false,
linkButton = false,
to,
leadingIcon,
trailingIcon,
...otherProps
} = props;

Expand Down Expand Up @@ -151,6 +153,27 @@ const Link = forwardRef( ( props, ref ) => {
const LinkComponent = getLinkComponent();
const ariaLabel = getAriaLabel();

// Set the prefix/suffix icons, based on the type of link this is and
// the props supplied.
let leadingIconToUse = leadingIcon;
let trailingIconToUse = trailingIcon;
Comment on lines +156 to +159
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO if an icon is passed as a prop, it should override the default icons that are related to specific kinds of links as below. It's fine for now but just a thought.


if ( back ) {
leadingIconToUse = <BackIcon width={ 14 } height={ 14 } />;
}

if ( external && ! hideExternalIndicator ) {
trailingIconToUse = <ExternalIcon width={ 14 } height={ 14 } />;
}

if ( arrow && ! inverse ) {
trailingIconToUse = <ArrowIcon width={ 14 } height={ 14 } />;
}

if ( arrow && inverse ) {
trailingIconToUse = <ArrowInverseIcon width={ 14 } height={ 14 } />;
}

return (
<LinkComponent
aria-label={ ariaLabel }
Expand All @@ -177,23 +200,17 @@ const Link = forwardRef( ( props, ref ) => {
to={ to }
{ ...otherProps }
>
{ back && (
{ !! leadingIconToUse && (
<IconWrapper marginRight={ 5 }>
<BackIcon width={ 14 } height={ 14 } />
{ leadingIconToUse }
</IconWrapper>
) }
<span>{ children }</span>
{ external && ! hideExternalIndicator && (
<span className="googlesitekit-cta-link__contents">
{ children }
</span>
{ !! trailingIconToUse && (
<IconWrapper marginLeft={ 5 }>
<ExternalIcon width={ 14 } height={ 14 } />
</IconWrapper>
) }
{ arrow && (
<IconWrapper marginLeft={ 5 }>
{ ! inverse && <ArrowIcon width={ 14 } height={ 14 } /> }
{ inverse && (
<ArrowInverseIcon width={ 14 } height={ 14 } />
) }
{ trailingIconToUse }
</IconWrapper>
) }
</LinkComponent>
Expand All @@ -212,11 +229,13 @@ Link.propTypes = {
hideExternalIndicator: PropTypes.bool,
href: PropTypes.string,
inverse: PropTypes.bool,
leadingIcon: PropTypes.node,
linkButton: PropTypes.bool,
onClick: PropTypes.func,
small: PropTypes.bool,
standalone: PropTypes.bool,
linkButton: PropTypes.bool,
to: PropTypes.string,
trailingIcon: PropTypes.node,
};

export default Link;
4 changes: 3 additions & 1 deletion assets/js/components/notifications/__snapshots__/ErrorNotifications.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@ exports[`ErrorNotifications renders the GTE message when the only unsatisfied sc
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Learn more
</span>
<span
Expand Down
24 changes: 14 additions & 10 deletions assets/js/components/settings/SettingsActiveModule/Footer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,13 +245,15 @@ export default function Footer( props ) {
__( 'Edit %s settings', 'google-site-kit' ),
name
) }
trailingIcon={
<PencilIcon
className="googlesitekit-settings-module__edit-button-icon"
width={ 10 }
height={ 10 }
/>
}
>
{ __( 'Edit', 'google-site-kit' ) }
<PencilIcon
className="googlesitekit-settings-module__edit-button-icon"
width="10"
height="10"
/>
</Link>
);
}
Expand All @@ -262,17 +264,19 @@ export default function Footer( props ) {
className="googlesitekit-settings-module__remove-button"
onClick={ handleDialog }
danger
trailingIcon={
<TrashIcon
className="googlesitekit-settings-module__remove-button-icon"
width={ 13 }
height={ 13 }
/>
}
>
{ sprintf(
/* translators: %s: module name */
__( 'Disconnect %s from Site Kit', 'google-site-kit' ),
name
) }
<TrashIcon
className="googlesitekit-settings-module__remove-button-icon"
width="13"
height="13"
/>
</Link>
);
} else if ( ! isEditing && moduleHomepage ) {
Expand Down
40 changes: 30 additions & 10 deletions assets/js/components/setup/__snapshots__/SetupUsingProxyWithSignIn.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ exports[`SetupUsingProxyWithSignIn should not render the Activate Analytics noti
class="googlesitekit-cta-link googlesitekit-header__logo-link"
href=""
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
<div
aria-hidden="true"
class="googlesitekit-logo"
Expand Down Expand Up @@ -77,7 +79,9 @@ exports[`SetupUsingProxyWithSignIn should not render the Activate Analytics noti
tabindex="-1"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Fix common issues
</span>
</a>
Expand All @@ -96,7 +100,9 @@ exports[`SetupUsingProxyWithSignIn should not render the Activate Analytics noti
tabindex="-1"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Read help docs
</span>
</a>
Expand All @@ -115,7 +121,9 @@ exports[`SetupUsingProxyWithSignIn should not render the Activate Analytics noti
tabindex="-1"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Get support
</span>
</a>
Expand Down Expand Up @@ -286,7 +294,9 @@ exports[`SetupUsingProxyWithSignIn should not render the Activate Analytics noti
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Google Privacy Policy.
</span>
<span
Expand Down Expand Up @@ -346,7 +356,9 @@ exports[`SetupUsingProxyWithSignIn should render the setup page, including the A
class="googlesitekit-cta-link googlesitekit-header__logo-link"
href=""
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
<div
aria-hidden="true"
class="googlesitekit-logo"
Expand Down Expand Up @@ -404,7 +416,9 @@ exports[`SetupUsingProxyWithSignIn should render the setup page, including the A
tabindex="-1"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Fix common issues
</span>
</a>
Expand All @@ -423,7 +437,9 @@ exports[`SetupUsingProxyWithSignIn should render the setup page, including the A
tabindex="-1"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Read help docs
</span>
</a>
Expand All @@ -442,7 +458,9 @@ exports[`SetupUsingProxyWithSignIn should render the setup page, including the A
tabindex="-1"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Get support
</span>
</a>
Expand Down Expand Up @@ -667,7 +685,9 @@ exports[`SetupUsingProxyWithSignIn should render the setup page, including the A
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Google Privacy Policy.
</span>
<span
Expand Down
5 changes: 3 additions & 2 deletions assets/js/components/user-input/UserInputPreviewGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,11 @@ export default function UserInputPreviewGroup( {
( !! currentlyEditingSlug && ! isEditing )
}
linkButton
trailingIcon={
<ChevronDownIcon width={ 20 } height={ 20 } />
}
>
{ __( 'Edit', 'google-site-kit' ) }

<ChevronDownIcon width={ 20 } height={ 20 } />
</Link>
</LoadingWrapper>
</div>
Expand Down
8 changes: 6 additions & 2 deletions assets/js/googlesitekit/widgets/components/__snapshots__/WPDashboardReportError.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ exports[`WPDashboardReportError should only render one error per module when the
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Get help
</span>
</a>
Expand Down Expand Up @@ -76,7 +78,9 @@ exports[`WPDashboardReportError should only render one error per module when the
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Get help
</span>
</a>
Expand Down
4 changes: 3 additions & 1 deletion ...modules/adsense/components/widgets/__snapshots__/ConnectAdSenseCTATileWidget.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ exports[`ConnectAdSenseCTATileWidget should render the Connect AdSense CTA tile
<button
class="googlesitekit-cta-link googlesitekit-cta-link--secondary"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Connect AdSense
</span>
</button>
Expand Down
16 changes: 12 additions & 4 deletions ...odules/analytics-4/components/common/__snapshots__/EnhancedMeasurementSwitch.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ exports[`EnhancedMeasurementSwitch should render correctly in the already enable
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Learn more
</span>
<span
Expand Down Expand Up @@ -81,7 +83,9 @@ exports[`EnhancedMeasurementSwitch should render correctly in the default state
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Learn more
</span>
<span
Expand Down Expand Up @@ -144,7 +148,9 @@ exports[`EnhancedMeasurementSwitch should render correctly in the disabled state
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Learn more
</span>
<span
Expand Down Expand Up @@ -198,7 +204,9 @@ exports[`EnhancedMeasurementSwitch should render correctly in the loading state
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Learn more
</span>
<span
Expand Down
16 changes: 12 additions & 4 deletions ...ents/dashboard/EnhancedMeasurementActivationBanner/__snapshots__/SetupBanner.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ exports[`SetupBanner should render correctly when the user does have the edit sc
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Learn more
</span>
<span
Expand Down Expand Up @@ -72,7 +74,9 @@ exports[`SetupBanner should render correctly when the user does have the edit sc
<button
class="googlesitekit-cta-link"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Maybe later
</span>
</button>
Expand Down Expand Up @@ -137,7 +141,9 @@ exports[`SetupBanner should render correctly when the user does not have the edi
rel="noopener noreferrer"
target="_blank"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Learn more
</span>
<span
Expand Down Expand Up @@ -165,7 +171,9 @@ exports[`SetupBanner should render correctly when the user does not have the edi
<button
class="googlesitekit-cta-link"
>
<span>
<span
class="googlesitekit-cta-link__contents"
>
Maybe later
</span>
</button>
Expand Down