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

[DST-111]: enhance styling tabs #3250

Merged
merged 9 commits into from
Aug 16, 2023
Merged

[DST-111]: enhance styling tabs #3250

merged 9 commits into from
Aug 16, 2023

Conversation

OsamaAbdellateef
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2023

🦋 Changeset detected

Latest commit: 18b229b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@marigold/components Minor
@marigold/system Minor
@marigold/theme-b2b Major
@marigold/theme-core Major
@marigold/storybook-config Patch
@marigold/docs Minor
@marigold/icons Patch
@marigold/theme-preset Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Aug 4, 2023

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

Name Status Preview Comments Updated (UTC)
marigold-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 16, 2023 5:14am
marigold-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 16, 2023 5:14am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
marigold-production ⬜️ Ignored (Inspect) Visit Preview Aug 16, 2023 5:14am

@github-actions github-actions bot added the type:feature New feature or component label Aug 4, 2023
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #3250 (18b229b) into main (13841ab) will not change coverage.
Report is 45 commits behind head on main.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #3250   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          112       112           
  Lines         7088      7108   +20     
  Branches       319       320    +1     
=========================================
+ Hits          7088      7108   +20     
Files Changed Coverage Δ
packages/components/src/Tabs/Context.ts 100.00% <100.00%> (ø)
packages/components/src/Tabs/TabPanel.tsx 100.00% <100.00%> (ø)
packages/components/src/Tabs/Tabs.tsx 100.00% <100.00%> (ø)
packages/system/src/types/theme.ts 100.00% <100.00%> (ø)

@sebald
Copy link
Member

sebald commented Aug 7, 2023

@OsamaAbdellateef Tests are missing! :)

packages/components/src/Tabs/TabPanel.tsx Outdated Show resolved Hide resolved
packages/components/src/Tabs/Tabs.tsx Outdated Show resolved Hide resolved
tab-1 content
</Tabs.Item>
<Tabs.Item title="tab2" key="2">
<Tabs.Item key="2" title={'tab2'}>
tab-2 content
Copy link
Member

@sarahgm sarahgm Aug 9, 2023

Choose a reason for hiding this comment

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

can I also set a variant on the <Tab.Panel> ? @OsamaAbdellateef

Copy link
Contributor Author

@OsamaAbdellateef OsamaAbdellateef Aug 9, 2023

Choose a reason for hiding this comment

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

no we can't , why should we pass variant for <Tabs.Item> ? @sarahgm

Copy link
Member

@sarahgm sarahgm Aug 9, 2023

Choose a reason for hiding this comment

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

don't know maybe we need this in future, or how can I set styles on the panel? @OsamaAbdellateef :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can style panel like this :
<Tabs.Item className='bg-red-300'> tab panel content to be styled </Tabs.Item>
so the className here is for tab panel not for the tab button
@sarahgm

Copy link
Member

@sarahgm sarahgm Aug 9, 2023

Choose a reason for hiding this comment

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

but how would a user than style it? @OsamaAbdellateef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean the tab button ?!
if that what u mean then idk
because className is for tab panel but what if you want to style the tab button ?! then you should make this className reserved for tab button instead of tab panel
what i mean here you only have one className not two then you either chose to style for tab panel or button ?

Copy link
Member

Choose a reason for hiding this comment

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

Ehhhh Now I'm also confused :D :D @sebald you as author of the issue can you explain to me? :D

Copy link
Member

Choose a reason for hiding this comment

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

We want the variant to be used by the container, tab, tablist and tabpanel :) Does this help?

@OsamaAbdellateef
Copy link
Contributor Author

@sarahgm
now i get the changes back and deleted className from components

@@ -1,6 +1,11 @@
import { createContext, useContext } from 'react';

export const TabContext = createContext<{
classNames: { tabs: string; tab: string };
classNames: {
Copy link
Member

Choose a reason for hiding this comment

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

do we than need this still? @OsamaAbdellateef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarahgm
const classNames = useClassNames({ component: 'Tabs', size, variant, });
<TabContext.Provider value={{ classNames }}>
instead of using useClassNames multiple times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarahgm
is it clear ?

@@ -10,9 +12,12 @@ export interface TabPanelProps extends AriaTabPanelProps {
export const TabPanel = ({ state, ...props }: TabPanelProps) => {
const ref = useRef(null);
const { tabPanelProps } = useTabPanel(props, state, ref);
const selectedItemProps = state.selectedItem?.props;
const { classNames } = useTabContext();
Copy link
Member

Choose a reason for hiding this comment

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

I think this we also don't need than, right @OsamaAbdellateef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarahgm
I think we need it, these classNames for styles via theme

sarahgm
sarahgm previously approved these changes Aug 15, 2023
@sarahgm sarahgm requested a review from sebald August 15, 2023 06:54
sebald
sebald previously approved these changes Aug 16, 2023
@sebald sebald dismissed stale reviews from sarahgm and themself via 18b229b August 16, 2023 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants