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: add CrossLink component #5781

Merged
merged 3 commits into from
Sep 26, 2023

Conversation

HinataKah0
Copy link
Contributor

@HinataKah0 HinataKah0 commented Sep 10, 2023

Description

Implement new CrossLink component and Storybook's story as specified in the Figma.

Validation

  1. Deploy Storybook
  2. Manual testing by comparing to Figma
  3. And check if Github pipeline passes
Screenshot 2023-09-26 at 8 45 17 PM

Related Issues

Fixes #5761

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo test to check if all tests are passing.
  • I've covered new added functionality with unit tests if necessary.

@vercel
Copy link

vercel bot commented Sep 10, 2023

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

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 26, 2023 3:15pm

@HinataKah0
Copy link
Contributor Author

Marked it as draft because there are still some things that I want to check/think first.

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

Good work ! Don't forget to give the box a maximum size

components/Common/CrossLink/index.tsx Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Sep 11, 2023

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 92%
90.24% (185/205) 75% (30/40) 86.66% (39/45)

Unit Test Report

Tests Skipped Failures Errors Time
14 0 💤 0 ❌ 0 🔥 3.605s ⏱️

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

The last thing for me is the background, you should try to make it transparent. Because on a dark theme it's white whereas in the figma the background is black. I'm not sure that's clear but I hope you've understood 😁.

@ovflowd
Copy link
Member

ovflowd commented Sep 12, 2023

The last thing for me is the background, you should try to make it transparent. Because on a dark theme it's white whereas in the figma the background is black. I'm not sure that's clear but I hope you've understood 😁.

I dont think the bg should be transparent. Should be white.

@AugustinMauroy
Copy link
Contributor

I dont think the bg should be transparent. Should be white.

on figma the background of box is same of app background so it's should be transparent.

@ovflowd
Copy link
Member

ovflowd commented Sep 12, 2023

I dont think the bg should be transparent. Should be white.

on figma the background of box is same of app background so it's should be transparent.

That's an overstatement. The Component Figma's CSS properties definitely declares a white background. This should not be transparent. First, if we add this to a non-white background we would already be non-compliant with the theme, same on dark mode.

@HinataKah0
Copy link
Contributor Author

Addressing @AugustinMauroy comments here

Don't forget to give the box a maximum size

Actually I am not sure what "maximum size" you're talking about. If you're talking about the max-height/width property, I don't think we need to specify it as there's no need for it.

If you're talking about the preview in Storybook, I followed the 305px described in the Figma to demonstrate that the text truncation works.

Transparent background

As Claudio has described, we have color schemes for both light and dark mode. It's better to state what background color is used for each component as per the Figma. So there are no surprises when we are using the components later.

@ovflowd
Copy link
Member

ovflowd commented Sep 25, 2023

FYI @HinataKah0 that we moved to PostCSS, so you need to rename the file from .scss to .css

Signed-off-by: Claudio W <cwunder@gnome.org>
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

I think we're ready! LGTM! Thank you, @HinataKah0 !!

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Sep 26, 2023
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Sep 26, 2023
@ovflowd ovflowd added this pull request to the merge queue Sep 26, 2023
Merged via the queue into nodejs:main with commit 4eca18b Sep 26, 2023
10 of 12 checks passed
@HinataKah0 HinataKah0 deleted the create-crosslink-component branch September 26, 2023 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create CrossLink component
10 participants