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(Tabs): Contained tabs on the grid #13927

Conversation

francinelucca
Copy link
Collaborator

@francinelucca francinelucca commented Jun 5, 2023

Closes #13353

Adds fullWidth prop to TabList so that Tabs (container) can be aligned to the Grid

Changelog

New

  • fullWidth prop now available for contained TabList
  • 'contained fullWidth' tab story and e2e test
  • tests for fullWidth prop and added mock for useMatchMedia in Tab testing
  • 'cds-tabs--full-width' class for fullWidth contained Tabs and accompanying style
  • Grid guidance using fullWidth prop for Tabs

Testing / Reviewing

  1. Check that stories match spec: Tabs: Aligning to the Grid design guidance #13351 (comment)
  2. Check developer guidance is true and coherent: https://deploy-preview-13927--carbon-components-react.netlify.app/?path=/docs/components-tabs--contained-full-width#tabs-and-the-grid---fullwidth-prop
  3. Should have no visual/behavioral regression to default tab styles and functionality outside of fullWidth

TODO:

  • Remove Test story

Co-authored-by: Guilherme Datilio Ribeiro <guilhermedatilio@gmail.com>
@netlify
Copy link

netlify bot commented Jun 5, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit fec0348
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6494a4c4dfce8d000811c719
😎 Deploy Preview https://deploy-preview-13927--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Jun 5, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit fec0348
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6494a4c4e901250008b2f45c
😎 Deploy Preview https://deploy-preview-13927--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

francinelucca and others added 2 commits June 7, 2023 13:03
…353-tabs-implement-contained-tabs-on-the-css-grid
Co-authored-by: Guilherme Datilio Ribeiro <guilhermedatilio@gmail.com>
@francinelucca
Copy link
Collaborator Author

We're still ironing some dev things out but this should be good for design review @kingtraceyj . You can check it out at this url: https://deploy-preview-13927--carbon-components-react.netlify.app/iframe.html?args=&id=components-tabs--contained-grid-test&viewMode=story

Copy link
Member

@kingtraceyj kingtraceyj left a comment

Choose a reason for hiding this comment

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

this is working as intended! 💥

francinelucca and others added 2 commits June 12, 2023 12:47
…353-tabs-implement-contained-tabs-on-the-css-grid
Co-authored-by: Guilherme Datilio Ribeiro <guilhermedatilio@gmail.com>
@francinelucca francinelucca changed the title WIP: feat(Tabs): Contained tabs on the grid feat(Tabs): Contained tabs on the grid Jun 12, 2023
@francinelucca francinelucca marked this pull request as ready for review June 12, 2023 17:37
@francinelucca francinelucca requested a review from a team as a code owner June 12, 2023 17:37
@francinelucca francinelucca requested review from andreancardona, guidari and tw15egan and removed request for guidari June 12, 2023 17:37
@guidari guidari self-assigned this Jun 12, 2023
@francinelucca francinelucca requested a review from a team as a code owner June 12, 2023 18:35
@alisonjoseph
Copy link
Member

alisonjoseph commented Jun 15, 2023

Question: are we not aligning individual tabs on the grid anymore? That was the original intent of the design spec, but maybe it changed?

Screenshot 2023-06-15 at 10 26 54 AM

This is a screen shot from the POC that we did as a comparison.
Screenshot 2023-06-15 at 10 32 37 AM

Edited to add it seems that this is intended 😅 Added some suggestions below on how we can possibly clarify the docs so they can get tabs aligned on the grid, and not just full width.

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

This is looking great! I think we can make the docs a little clearer by adding / showing an example on how to actually get the tabs on the grid by matching the column numbers. We might want to show that in the code sample on the docs also.

packages/react/src/components/Tabs/Tabs.mdx Show resolved Hide resolved
guidari and others added 2 commits June 16, 2023 15:13
Co-authored-by: Guilherme Datilio Ribeiro <guilhermedatilio@gmail.com>
@francinelucca
Copy link
Collaborator Author

@alisonjoseph yeah we confirmed with Tracey & Lauren only the Container needs to be aligned to the Grid and if the number of columns coincide with the number of Tabs then the individual tab items will also be aligned to the Grid. We just added some extra guidance around that in the Docs, let us know what you think :)

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM, and a lot easier to understand now with added docs 🥳

… github.com:francinelucca/carbon into 13353-tabs-implement-contained-tabs-on-the-css-grid
@kodiakhq kodiakhq bot merged commit 11c70f3 into carbon-design-system:main Jun 22, 2023
15 checks passed
@francinelucca francinelucca deleted the 13353-tabs-implement-contained-tabs-on-the-css-grid branch June 22, 2023 20:48
@carbon-bot
Copy link
Contributor

Hey there! v11.32.0 was just released that references this issue/PR.

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

Successfully merging this pull request may close these issues.

Tabs: Implement contained tabs on the css grid
6 participants