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

1261 introduce new start screen with helpful links #1327

Merged
merged 8 commits into from
Oct 10, 2022
Merged

1261 introduce new start screen with helpful links #1327

merged 8 commits into from
Oct 10, 2022

Conversation

MightyDevKnight
Copy link
Contributor

ScreenShot_20221007152411
ScreenShot_20221007152653

@vercel
Copy link

vercel bot commented Oct 7, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
figma-tokens ✅ Ready (Inspect) Visit Preview Oct 9, 2022 at 5:51PM (UTC)
ft-storybook ✅ Ready (Inspect) Visit Preview Oct 9, 2022 at 5:51PM (UTC)

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2022 •
edited
Loading

Commit SHA:9af4cc1716b7fe8583bd4c14d662d688c8b68bb1

Test coverage results 🧪

Code coverage diff between base branch:next and head branch: 1261-introduce-new-start-screen-with-helpful-links
Status File % Stmts % Branch % Funcs % Lines
🟢 total 71.25 (0) 63.16 (0) 68.35 (0) 71.57 (0.01)
🟢 src/app/components/StartScreen.tsx 61.54 (11.54) 100 (0) 33.33 (0) 61.54 (11.54)

six7
six7 requested changes Oct 7, 2022
View reviewed changes
cypress/integration/startup.spec.js Outdated
@@ -64,6 +64,6 @@ describe('Loads application', () => {
it('shows welcome page when no token values are given', () => {
cy.visit('/');
cy.startup(mockStartupParams);
cy.get('p').contains('Welcome to Figma Tokens');
cy.get('a').contains("Found an issue We're on GitHub!");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cy.get('a').contains("Found an issue We're on GitHub!");
cy.get('a').contains("Found an issue? We're on GitHub!");

src/app/components/Callout.tsx Outdated
<Stack direction="row" gap={2}>
<Box css={{ color: '$interaction' }}>
<IconBell />
<Box css={{ backgroundColor: '$startScreenAlertBg', padding: '$4', borderRadius: '$default' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This component can be used outside Start screen as well, lets not introduce tokens that bind the usage to the start screen.

Suggested change
<Box css={{ backgroundColor: '$startScreenAlertBg', padding: '$4', borderRadius: '$default' }}>
<Box css={{ backgroundColor: '$dangerBg', padding: '$4', borderRadius: '$default' }}>

src/app/components/Callout.tsx Outdated
<IconBell />
<Box css={{ backgroundColor: '$startScreenAlertBg', padding: '$4', borderRadius: '$default' }}>
<Stack direction="row" gap={4}>
<Box css={{ color: '$dangerBg', marginTop: '$3' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're applying a token to the foreground color of an object, applying a Background token for that is not good.

Suggested change
<Box css={{ color: '$dangerBg', marginTop: '$3' }}>
<Box css={{ color: '$dangerFg', marginTop: '$3' }}>

src/app/components/Callout.tsx Outdated
</Box>
<Stack align="start" direction="column" gap={2}>
<Heading>{heading}</Heading>
<Text size="xsmall">{description}</Text>
<StyledButton data-cy={id} type="button" onClick={action.onClick}>
<Button data-cy={id} id="button-configure" size="small" variant="primary" onClick={action.onClick}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This component can be used outside Start screen as well, let's not bind it to it. The ID should be dynamic.

src/app/components/StartScreen.tsx Outdated
height: '55px',
});

const HelpfulLinks = styled('a', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would call this singular HelpfulLink or so.

src/app/components/StartScreen.tsx Outdated
</a>
<Stack direction="column" gap={4} css={{ padding: '2rem', background: '$startScreenBg' }}>
<Stack direction="row" gap={2}>
<StyledFigmaMark />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you call this 'FigmaTokensLogoMark' and 'FigmaTokensLogo'? This is not the Figma Mark but our logo.

src/app/components/StartScreen.tsx Outdated
'&:hover, &:focus': {
color: '$onInteraction',
},
rel: 'noreferrer',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a html property, not a css one.

Suggested change
rel: 'noreferrer',

src/app/components/StartScreen.tsx Outdated
{/* eslint-disable-next-line */}
<img alt="Figma Tokens Splashscreen" src={require('../assets/tokens-intro.jpg')} className="rounded" />
</a>
<Stack direction="column" gap={4} css={{ padding: '2rem', background: '$startScreenBg' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Stack direction="column" gap={4} css={{ padding: '2rem', background: '$startScreenBg' }}>
<Stack direction="column" gap={4} css={{ padding: '2rem', background: '$bgSubtle', justifyContent: 'center' }}>

This is what it looked like for me in Light mode.
image

src/app/components/StartScreen.tsx Outdated
@@ -32,40 +57,64 @@ function StartScreen() {

return (
<div className="h-auto p-4 my-auto content scroll-container">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<div className="h-auto p-4 my-auto content scroll-container">
<div className="h-auto p-4 content scroll-container">

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also change this container to have

height: 100%
display: flex

src/app/components/StartScreen.tsx
@@ -32,40 +57,64 @@ function StartScreen() {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also fix that on line 49 we currently have
dispatch.uiState.setActiveTab(Tabs.TOKENS)
this should be
dispatch.uiState.setActiveTab(Tabs.SETTINGS)

Right now when you click Enter credentials you stay on start screen but it should take you to settings. which the above suggestion does

@six7
Copy link
Collaborator

six7 commented Oct 7, 2022

@MightyDevKnight can you link the issue?

@MightyDevKnight MightyDevKnight linked an issue Oct 8, 2022 that may be closed by this pull request
Introduce new Start screen with helpful links #1261
Closed
six7
six7 approved these changes Oct 10, 2022
View reviewed changes
Copy link
Collaborator

@six7 six7 left a comment

Choose a reason for hiding this comment

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

Nice 🎉

@six7 six7 merged commit d415926 into next Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LiamMartens LiamMartens LiamMartens approved these changes

@six7 six7 six7 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce new Start screen with helpful links
3 participants
@MightyDevKnight @six7 @LiamMartens