Skip to content

feat(pride logo): introduce #6734

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

Merged
merged 3 commits into from
Jun 1, 2024
Merged

feat(pride logo): introduce #6734

merged 3 commits into from
Jun 1, 2024

Conversation

AugustinMauroy
Copy link
Member

Description

Read issues #6733 to understand what is going on.

Validation

Capture d’écran 2024-05-24 à 23 25 37 Capture d’écran 2024-05-24 à 23 25 25

Related Issues

Related to #6733

Check List

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

@AugustinMauroy AugustinMauroy requested review from a team as code owners May 24, 2024 21:26
Copy link

vercel bot commented May 24, 2024

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 31, 2024 7:14am

Copy link
Contributor

github-actions bot commented May 24, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 99 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 98 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 94 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 100 🟢 92 🔗

Copy link
Contributor

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 91%
90.06% (589/654) 76.08% (175/230) 92.24% (119/129)

Unit Test Report

Tests Skipped Failures Errors Time
128 0 💤 0 ❌ 0 🔥 5.884s ⏱️

Copy link
Member

Choose a reason for hiding this comment

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

What I'm trying to say is that you can merge all the Node.js logo variants into a single React Component, they are after all a React Component.

This (what you did below) is not what I was referring to, and does not reduce the duplication of code.

Try to reflect a bit on how to achieve what I'm saying. It's pretty simple actually ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

I thinks I go your idea just just want one nodejsLogo stroke of text will be apply with tailwind and two other component for Hexagon

@bmuenzenmeyer
Copy link
Collaborator

I'm thinking we get this released sooner rather than later, even if we aren't in complete alignment on the approach.

@bmuenzenmeyer bmuenzenmeyer dismissed araujogui’s stale review June 1, 2024 21:39

We want to release this ASAP. We can refactor at any time after this. But pride month has started

Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

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

Content over purity of engineering

@bmuenzenmeyer bmuenzenmeyer merged commit d09e4f1 into main Jun 1, 2024
5 checks passed
@bmuenzenmeyer bmuenzenmeyer deleted the prideeeee branch June 1, 2024 21:43
@bmuenzenmeyer
Copy link
Collaborator

ugh - on mobile this all looked green - something's wrong post-merge - looking into it

@AugustinMauroy
Copy link
Member Author

IMG_0676
@bmuenzenmeyer ?

@AugustinMauroy
Copy link
Member Author

I thinks It’s small delay from Vercel deployment

@bmuenzenmeyer
Copy link
Collaborator

@AugustinMauroy
Copy link
Member Author

Oh my bad I thinks you talk about logo 😅

@bmuenzenmeyer
Copy link
Collaborator

i'm glad its not outright broken, but apologies on any instability i rushed through - mobile is clearly not a good way to review CI status checks - thought i see here everything looks green.

i suggest we simply roll forward with a merge of a dependency update or something

@nodejs nodejs locked and limited conversation to collaborators Jun 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants