Skip to content

Conversation

@MichaelSun48
Copy link
Contributor

This PR makes non-visible changes to the tabs component in anticipation of future changes being made for custom views. Specifically, it separates the <Tab/> and <BaseTab/> variants and adds a new variant prop that controls which group of styles are being applied to the baseTab component.

It also reorganizes the component structure of the <TabList> to allow for the overflow menu to be reusable, since it will be used in the future draggable tabs component.

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 24, 2024
@MichaelSun48 MichaelSun48 marked this pull request as ready for review July 24, 2024 17:29
@MichaelSun48 MichaelSun48 requested a review from a team July 24, 2024 17:29
@codecov
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 71.87500% with 9 lines in your changes missing coverage. Please review.

Project coverage is 78.11%. Comparing base (071f728) to head (4e3a182).
Report is 192 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #74867      +/-   ##
==========================================
- Coverage   78.16%   78.11%   -0.05%     
==========================================
  Files        6733     6740       +7     
  Lines      300307   300637     +330     
  Branches    51649    51726      +77     
==========================================
+ Hits       234729   234842     +113     
- Misses      59255    59501     +246     
+ Partials     6323     6294      -29     
Files Coverage Δ
static/app/components/interactionStateLayer.tsx 100.00% <100.00%> (ø)
static/app/components/tabs/tabList.tsx 88.88% <83.33%> (+0.31%) ⬆️
static/app/components/tabs/tab.tsx 81.63% <68.00%> (-15.34%) ⬇️

... and 293 files with indirect coverage changes

Copy link
Member

@malwilley malwilley 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 add a section to the .stories file so we can see what the new variant looks like?

@MichaelSun48
Copy link
Contributor Author

MichaelSun48 commented Jul 24, 2024

Could you add a section to the .stories file so we can see what the new variant looks like?

Done!

image

Copy link
Member

@malwilley malwilley left a comment

Choose a reason for hiding this comment

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

Is it purposeful that there is a large gap between tabs?

CleanShot 2024-07-24 at 13 25 40

@MichaelSun48
Copy link
Contributor Author

MichaelSun48 commented Jul 24, 2024

Is it purposeful that there is a large gap between tabs?

@malwilley
Nope, this was caused by the fact that the padding is still applied to the tab names even if they are not selected. This is fixed in the next commit!

image

@MichaelSun48 MichaelSun48 force-pushed the msun/addNewTabVariant branch from 932362f to f59efbc Compare July 24, 2024 23:59
Comment on lines +88 to +90
InteractionStateLayer.defaultProps = {
hasSelectedBackground: true,
};
Copy link
Contributor Author

@MichaelSun48 MichaelSun48 Jul 25, 2024

Choose a reason for hiding this comment

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

This is how default props have to be defined apparently 🫠 (setting hasSelectedBackground=true in the destructed props does not work)

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, because it's a styled component I guess

Comment on lines +88 to +90
InteractionStateLayer.defaultProps = {
hasSelectedBackground: true,
};
Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, because it's a styled component I guess

* Additional props to be merged with `tabProps`. This is used
* by <DraggableTab> to pass in props used for drag-and-drop functionality.
*/
additionalProps?: React.HTMLAttributes<HTMLElement>;
Copy link
Member

@malwilley malwilley Jul 25, 2024

Choose a reason for hiding this comment

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

Can we remove additionalProps by merging these in DraggableTab instead?

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 remember trying to do that but I vaguely remember there being an issue that forced me to pass in props here. I think a good temporary solution would be to remove additionalProps for now and revisit that problem as part of merging in the changes I made a couple weeks ago.

Copy link
Member

@malwilley malwilley 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!

@MichaelSun48 MichaelSun48 merged commit fc893af into master Jul 25, 2024
@MichaelSun48 MichaelSun48 deleted the msun/addNewTabVariant branch July 25, 2024 20:23
Christinarlong pushed a commit that referenced this pull request Jul 26, 2024
This PR makes non-visible changes to the tabs component in anticipation
of future changes being made for custom views. Specifically, it
separates the `<Tab/>` and `<BaseTab/>` variants and adds a new
`variant` prop that controls which group of styles are being applied to
the baseTab component.

It also reorganizes the component structure of the `<TabList>` to allow
for the overflow menu to be reusable, since it will be used in the
future draggable tabs component.
MichaelSun48 added a commit that referenced this pull request Jul 26, 2024
This PR creates the `<DraggableTabs/>` component along with many
supporting components. It utilizes the new tab variant [recently
introduced](#74867), and adds
support for reordering them via dragging (implemented w/ Framer motion).

This PR contains a subset of changes from a previous
[PR](#73543) that contained a
lot of new changes to the draggable tabs component, and it will be the
first in a series of PRs that break up that PR into smaller chunks. This
PR _should_ be the largest of them all, since it creates all of the
draggable tabs components. I have tried to pare the old PR down to
_only_ contain the changes necessary to get the draggable tabs to be
actually draggable (using Framer motion), but please feel free to
comment on anything that looks like it may be an artifact of these
changes.

Known issues in this current implementation that will be fixed in
subsequent PRs:

- There are no tests for this component 
- Overflow tabs interact with the tab dividers very weirdly 
- Tab names wrap very weirdly 


https://github.com/user-attachments/assets/329fe197-b6af-47af-8faa-4d34ef65181f
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants