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: new styles for subtask in lab #2033

Merged
merged 5 commits into from
Feb 7, 2022
Merged

feat: new styles for subtask in lab #2033

merged 5 commits into from
Feb 7, 2022

Conversation

kisielewskik
Copy link
Contributor

@kisielewskik kisielewskik commented Jan 26, 2022

Description

Task from Alex Ronquillo from New Relic org. Purpose of this is making new styles for lab procedures while we have parrarel sub procedures.

Reviewer Notes

For testing it you have to make procedure with not-integer ProdIdx, example: ProcIdx 2.1, should be under ProcIdx 2 with simpler styles.

@kisielewskik kisielewskik requested review from a team, zstix and josephgregoryii and removed request for a team January 26, 2022 11:39
@CLAassistant
Copy link

CLAassistant commented Jan 26, 2022

CLA assistant check
All committers have signed the CLA.

Comment on lines 51 to 53
const procIdxisNotInteger = Number.isInteger(
frontmatter.procIdx
);
Copy link
Contributor

Choose a reason for hiding this comment

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

as this is set up now procIdxisNotInteger will evaluate to true when a number is an integer. would you mind renaming it to procIdxIsInteger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed name of variable is more logic now

Comment on lines 23 to 24
width: ${procIdxIsInteger ? '100%' : '90%'};
margin-left: ${procIdxIsInteger ? '0' : '10%'};
Copy link
Contributor

Choose a reason for hiding this comment

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

changing the styles here have had some unintended consequences in other places that we use the GuideTiles. I think moving these styles to the LabOverviewTemplate might be best. you can use the css prop within that guidetile just like here and select for the p and h3

normal styling:
Screen Shot 2022-02-04 at 9 15 26 AM

current changes:
Screen Shot 2022-02-04 at 9 15 34 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

That spacing between the tiles seems really wide I'd recommend following the pattern we use for space between the quickstart tiles on I/O

https://developer.newrelic.com/instant-observability/
Screen Shot 2022-02-04 at 11 00 32 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totalny agree, I didn’t noticed that change will have so big impact on other views, i will move that styles to LabOverviewTemplate asap to keep website pattern, thank you for your advices

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @kisielewskik!

Comment on lines 23 to 24
width: ${procIdxIsInteger ? '100%' : '90%'};
margin-left: ${procIdxIsInteger ? '0' : '10%'};
Copy link
Contributor

Choose a reason for hiding this comment

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

That spacing between the tiles seems really wide I'd recommend following the pattern we use for space between the quickstart tiles on I/O

https://developer.newrelic.com/instant-observability/
Screen Shot 2022-02-04 at 11 00 32 AM

jpvajda
jpvajda previously requested changes Feb 4, 2022
}
/>
))}
.map(({ fields, frontmatter }, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Attached is the screen shot for this change so it's visible, this is OK for me, so I approve this.
Screen Shot 2022-02-04 at 9 47 45 AM

@jpvajda jpvajda self-assigned this Feb 4, 2022
@LizBaker LizBaker dismissed jpvajda’s stale review February 7, 2022 17:13

changes addressed

@LizBaker LizBaker merged commit d38eb9b into newrelic:develop Feb 7, 2022
@nr-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 1.77.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants