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

[core] feat: Section component #6245

Merged
merged 19 commits into from
Jul 11, 2023
Merged

Conversation

CPerinet
Copy link
Contributor

@CPerinet CPerinet commented Jun 26, 2023

Screenshot 2023-06-27 at 10 42 13 PM

This was referenced Jun 26, 2023
@CPerinet CPerinet requested a review from adidahiya June 27, 2023 20:50
@CPerinet CPerinet marked this pull request as ready for review July 4, 2023 09:48
@adidahiya
Copy link
Contributor

@CPerinet I cleaned up some of the code based on my review.

  • Instead of "small" appearance, I've switched to using the newer "compact" modifier (this exists for HTMLTable and Tooltip components as well, and is better suited to reduced padding visuals).
  • I understand that <Card> is being used inside <Section>, but I feel like it doesn't really make sense to use CardProps here since we're not allowing any of the props which are unique/specific to that component. Better to define SectionProps by inheriting from the same underlying interfaces which CardProps does.
  • We might want to consider further breaking down the component to also define a new <SectionHeader> component. This might help to disambiguate some of the prop names (for example, rightElement)
  • There are still some edge cases for some combinations of props that need to be ironed out:
    • tabs defined & compact={true}
      image
    • tabs defined & collapsible={true}
      2023-07-05 17 22 47

@adidahiya
Copy link
Contributor

Re: edge case where tabs & collapsible are both enabled. We can work around this by making these props mutually exclusive. tabDefinitions={...} collapsible={true} would be unsupported.

Also, I think the collapse feature should actually use the <Collapse> component so that we get the nice transition animation.

@adidahiya
Copy link
Contributor

Note that I updated the docs page a bit and fixed the console error related to duplicate docs headings (you can't have two instances of the same heading in a .md page, like ## Props).

Latest docs preview build here

image

@adidahiya
Copy link
Contributor

I've removed Tabs support for now. It has some complications which we need to address, and we don't need to implement all the features in one PR. We can bring back that feature in a separate PR.

@adidahiya
Copy link
Contributor

  • We might want to consider further breaking down the component to also define a new <SectionHeader> component. This might help to disambiguate some of the prop names (for example, rightElement)

I tried this out and it turns out to be kind of cumbersome to implement. The SectionHeader would need to know a lot of the state that Section has access to, specifically collapsible and isCollapsed. So I think the current approach is fine, at least for now.

@adidahiya adidahiya changed the title Section [core] feat: Section component Jul 11, 2023
@@ -213,6 +214,17 @@ export const MULTISTEP_DIALOG_RIGHT_PANEL = `${MULTISTEP_DIALOG}-right-panel`;
export const MULTISTEP_DIALOG_NAV_TOP = `${MULTISTEP_DIALOG}-nav-top`;
export const MULTISTEP_DIALOG_NAV_RIGHT = `${MULTISTEP_DIALOG}-nav-right`;

export const SECTION = `${NS}-section`;
export const SECTION_COLLAPSED = `${SECTION}-collapsed`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to add a "section"-namespaced "collapsed" class name for now rather than a .bp5-collapsed name.

packages/core/src/components/section/sectionPanel.tsx Outdated Show resolved Hide resolved
align-items: start;
}

.metadata-panel {
Copy link
Contributor

Choose a reason for hiding this comment

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

@CPerinet for future reference: styling of example content should be done in CSS like this

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.

2 participants