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

(#1583) CTA Strip Storybook Update #1623

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

karlikpj
Copy link
Contributor

@karlikpj karlikpj commented Mar 28, 2024

Closes #1583

Copy link

@karlikpj karlikpj force-pushed the ticket/1583-ctastrip-storybook-update branch from e10f300 to 3f4325a Compare March 28, 2024 16:10
@karlikpj karlikpj marked this pull request as ready for review March 28, 2024 16:29
@andyvanavery31 andyvanavery31 requested a review from a team April 3, 2024 18:04
Copy link

@andyvanavery31 andyvanavery31 left a comment

Choose a reason for hiding this comment

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

Could you please change the symbol in front of Test Cases to be the folder icon, not the 2x2 grid? Also, can you please add a space between "TestCases" so that it reads "Test Cases"

2 questions, not change requests: 1. does the mobile breakpoint need left and right padding on the <div class="grid-container"? In use, we have 16px of padding inside the <div class="nci-hero__nci-cta-strip-container">, but I'm not sure if this just doesn't show up because it's not in that container.

  1. Should the images for mobile large and tablet be considered redundant? The buttons are the same width, just the width of the container changes with the breakpoint. If they are redundant, lets remove the one size from each (Default, Default Long, Default Spanish)

@karlikpj karlikpj force-pushed the ticket/1583-ctastrip-storybook-update branch 2 times, most recently from 8b61f9b to 99e441c Compare April 4, 2024 17:40
@karlikpj
Copy link
Contributor Author

karlikpj commented Apr 4, 2024

Q1 - we dont show any padding in the docsite/example code so Id opt not to do that in the regression images.
Q2 - each of those breakpoints has something different tablet switches the css to display:block and we have different padding in each

	@include at-media('mobile-lg') {
		padding: units(2.5) units(10);
	}

	@include at-media('tablet') {
		padding: units(2) units('card');
	}

	@include at-media('tablet-lg') {
		padding: units(2) units(10);
	}

	@include at-media('desktop') {
		padding: units(2) units(6);
	}
	```

@andyvanavery31 andyvanavery31 self-requested a review April 4, 2024 19:38
Copy link

@andyvanavery31 andyvanavery31 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@karlikpj karlikpj force-pushed the ticket/1583-ctastrip-storybook-update branch 5 times, most recently from 9af7998 to f4a514a Compare April 11, 2024 17:11
@karlikpj karlikpj force-pushed the ticket/1583-ctastrip-storybook-update branch 2 times, most recently from 0d5077d to 33f2601 Compare April 15, 2024 21:36
@olitharp-nci olitharp-nci force-pushed the ticket/1583-ctastrip-storybook-update branch from 33f2601 to 6fce3b5 Compare April 16, 2024 15:21
@olitharp-nci olitharp-nci merged commit 88b4ecb into develop Apr 16, 2024
3 checks passed
@olitharp-nci olitharp-nci deleted the ticket/1583-ctastrip-storybook-update branch April 16, 2024 15:42
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.

Update storybook structure for nci-cta-strip
3 participants