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

update lifecycle component to accept strings + commas #4919

Merged
merged 7 commits into from
Feb 14, 2024
Merged

update lifecycle component to accept strings + commas #4919

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
24312b3
update to accept strings + commas
mirnawong1 Feb 14, 2024
487b158
test pr
mirnawong1 Feb 14, 2024
c44b3e8
Merge branch 'current' into test
mirnawong1 Feb 14, 2024
ee1212f
fold in jason's feedback
mirnawong1 Feb 14, 2024
a651fec
Merge branch 'current' into test
mirnawong1 Feb 14, 2024
dcfd679
adjust if check and add component variations to test build
JKarlavige Feb 14, 2024
a587301
revert test changes
JKarlavige Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions website/docs/docs/use-dbt-semantic-layer/quickstart-sl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ meta:
api_name: dbt Semantic Layer APIs
---

### test <Lifecycle status="team,enterprise"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tessting to see if build comes out right. will remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
### test <Lifecycle status="team,enterprise"/>


import CreateModel from '/snippets/_sl-create-semanticmodel.md';
import DefineMetrics from '/snippets/_sl-define-metrics.md';
import ConfigMetric from '/snippets/_sl-configure-metricflow.md';
Expand Down
6 changes: 4 additions & 2 deletions website/src/components/lifeCycle/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ export default function Lifecycle(props) {
if (!props.status || (Array.isArray(props.status) && props.status.length === 0)) {
return null;
}
// Check if props.status is an array or a single value
const statuses = Array.isArray(props.status) ? props.status : [props.status];
// Check if props.status is an array or a single value amd to handle strings separated by commas
const statuses = typeof props.status ==='string'
?props.status.split(',').map(s => s.trim())
: 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.

Now that we're always passing in a string, there's a couple adjustments we can make:

You're on the right track with using .split(',')! I recommend updating this section and simply setting const statuses = props?.status?.split(','). This will always create an array, even if one item is passed in without a comma.

For example, passing in status="enterprise" will set props?.status?.split(',') to ['enterprise']. If the status prop isn't set on a component, statuses will become undefined.

I then recommend moving this line above the if check on line 22. We can then update the if check to: if(!statuses?.length). This will check if statuses has a value and is an array. If not, return null. Otherwise apply the html.

Let me know if you have any questions!

Copy link
Contributor Author

@mirnawong1 mirnawong1 Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ok awesome! thank you so much @JKarlavige ! i made those changes. one thing i kept though was

  if (!props.status || (Array.isArray(props.status) && props.status.length === 0)) {
    return null;
  }

because if i didn't add a status, the header would display a dash:
Screenshot 2024-02-14 at 18 29 14

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch, was able to recreate when passing an empty string into the status prop: status="". I merged your if check into the updated one so we only have 1 if check. The !props.status is enough to verify if status is an empty string.

Added the component to a few headers to double-check the build one more time, if the build passes i'll get this merged!


return (
<>
Expand Down
Loading