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

adding more usecases to lifecycle component #4804

Merged
merged 12 commits into from
Feb 10, 2024
Merged

adding more usecases to lifecycle component #4804

merged 12 commits into from
Feb 10, 2024

Conversation

mirnawong1
Copy link
Contributor

@mirnawong1 mirnawong1 commented Jan 25, 2024

this pr enhances the existing lifecycle component to add more badge support:

  • support for plans
  • support for lifecycle (same)
  • support for mulitiple badges
  • set custom colors
  • use fetElements to ignore lifecycle css in the right toc

see asana task for more info

*note, still waiting for color and add'l use case before final review.

way to use it

<Lifecycle status="enterprise" /> // single status, predefined color
<Lifecycle status={['team', 'enterprise']} /> // multiple statuses, predefined colors
<Lifecycle status="custom" backgroundColor="#123456" /> // Custom color

colors

predefined colors are:

  • prev used #f3f4f6 for the plan use case (enterprise, team), however now (as of 2/9, using #EBEDF0)
  • #047377 for the lifecycle use case (new, beta, ga, public preview)

@mirnawong1 mirnawong1 requested a review from a team as a code owner January 25, 2024 20:10
Copy link

vercel bot commented Jan 25, 2024

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

Name Status Preview Comments Updated (UTC)
docs-getdbt-com ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 9, 2024 11:45pm

@github-actions github-actions bot added size: small This change will take 1 to 2 days to address Docs team Authored by the Docs team @dbt Labs labels Jan 25, 2024
@github-actions github-actions bot added size: medium This change will take up to a week to address and removed size: small This change will take 1 to 2 days to address labels Jan 25, 2024
@mirnawong1
Copy link
Contributor Author

mirnawong1 commented Feb 7, 2024

hey @JKarlavige whenever you have a chance, can you review this PR please? i believe @nghi-ly and i were asking Jey to clarify the 'gray' color as it's looking washed out when you view the docs in light mode. the hex i used it #f3f4f6

@mirnawong1
Copy link
Contributor Author

mirnawong1 commented Feb 7, 2024

adjusted font per feedback
Screenshot 2024-02-07 at 13 18 04
Screenshot 2024-02-07 at 13 18 08

Copy link
Collaborator

@JKarlavige JKarlavige left a comment

Choose a reason for hiding this comment

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

Great work @mirnawong1! Left some feedback, let me know if you have any questions! Especially around the TOC side of it as it might get confusing.

Also, in the task there's a note: Support adding badge to page level (h1). Should this be included within this PR, or are we holding off on that for now?

website/src/theme/DocItem/Layout/index.js Outdated Show resolved Hide resolved
website/src/components/lifeCycle/index.js Outdated Show resolved Hide resolved
website/src/components/lifeCycle/index.js Show resolved Hide resolved
<span className={styles.lifecycle}>{props.status}</span>
);
// Check if props.status is an array or a single value
const statuses = Array.isArray(props.status) ? props.status : [props.status];
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 great way to ensure statuses is always an array!

Also wanted to share a shorthand way of achieving the same result using the spread operator:
const statuses = [...props.status];

If props.status is a string, it will spread it into an array. If props.status is an array, it will create a clone of it, essentially being the same as the initial array.

website/src/components/lifeCycle/index.js Show resolved Hide resolved
@mirnawong1
Copy link
Contributor Author

thanks so much @JKarlavige ! will review this 🙏

@mirnawong1
Copy link
Contributor Author

amazing! thank you so much -- @JKarlavige and @nghi-ly this is ready for re-review please

loom here: https://www.loom.com/share/b7a0482788674381a70574c9fd0bd503?sid=13b7add0-583b-4c98-a696-23b2d8768373

@mirnawong1
Copy link
Contributor Author

i've also updated the asana task and asked for color verification: https://app.asana.com/0/1200099998847559/1206377128451074/f

@JKarlavige JKarlavige self-requested a review February 9, 2024 15:37
Copy link
Collaborator

@JKarlavige JKarlavige left a comment

Choose a reason for hiding this comment

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

lgtm! :shipit: all edits look to work great, nice work getting those adjustments in!

Re: H1s - good call on adding the # H1 header to pages where this will be needed and adding the lifecycle component to that line. I think that's much cleaner and less lift than trying to support adding the lifecycle to the title from the frontmatter.

Awesome work! 😁

Comment on lines +22 to +24
if (!props.status || (Array.isArray(props.status) && props.status.length === 0)) {
return null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔥

@mirnawong1
Copy link
Contributor Author

yayyyy! thank you soo much @JKarlavige !! am going to wait for @nghi-ly to come online for next steps wrt to merging 🙏

Copy link
Contributor

@nghi-ly nghi-ly left a comment

Choose a reason for hiding this comment

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

lgtm!

re: H1s, just to double-check, will using # h1 title to override in the MD affect anything adversely like search results, SEO, google search results text, etc?

@nghi-ly nghi-ly merged commit 09678cc into current Feb 10, 2024
7 checks passed
@nghi-ly nghi-ly deleted the update-lifecycle branch February 10, 2024 00:19
@mirnawong1
Copy link
Contributor Author

mirnawong1 commented Feb 13, 2024

i'll defer to @JKarlavige on this but after researching, it looks like search engines might give more weight to the original frontmatter title than the overridden <H1> content. so would it make sense to use the #h1 tag (which will have the same content as the frontmatter title) for visual rendering and styling purposes? also generally the lifecycle status will get removed after a particular time so i'm not sure if we want that or not in the SEO?

@JKarlavige
Copy link
Collaborator

i'll defer to @JKarlavige on this but after researching, it looks like search engines might give more weight to the original frontmatter title than the overridden <H1> content. so would it make sense to use the #h1 tag (which will have the same content as the frontmatter title) for visual rendering and styling purposes? also generally the lifecycle status will get removed after a particular time so i'm not sure if we want that or not in the SEO?

The title frontmatter sets the <title> meta tag. This holds more importance over the h1 tag, as it's what shows in search results and in social sharing, and helps search engines know what the page is about.

The h1 tag provides additional context for search engines as to what the content on the page is related to.

Even if the frontmatter title property is different than the h1, as long as they accurately represent what the page is about there shouldn't be an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto update Docs team Authored by the Docs team @dbt Labs February-2024 size: medium This change will take up to a week to address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants