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] fix(SectionCard): rename component, fix Sass variable names #6272

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

adidahiya
Copy link
Contributor

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • Rename SectionPanel to SectionCard
  • Rename Sass variables in _section.scss to match naming used in other parts of the Sass codebase (_tooltip.scss, button/_common.scss)
  • feat(Section): add support for elevation prop which accepts a subset of the possible Elevation values (ZERO and ONE)

Reviewers should focus on:

Naming for the SectionCard component. It's a bit tricky to name container elements like this, but I feel like this is probably the best option for now. It's not:

  • a panel, since those can be more complex components with different kinds of content inside (we expect Section contents to be a little simpler). also, this creates some ambiguity with PanelStack.
  • "content" (as was the initial name in [core] feat: Section component #6245), since "content" is provided by Blueprint users, not Blueprint itself. the component is a container for content

references:

Screenshot

image

@adidahiya adidahiya merged commit 68d563c into develop Jul 13, 2023
@adidahiya adidahiya deleted the ad/section-card branch July 13, 2023 12:36
*/
export const SectionPanel: React.FC<SectionPanelProps> = React.forwardRef((props, ref) => {
export const SectionCard: React.FC<SectionCardProps> = React.forwardRef((props, ref) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The file itself was not renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! thankfully that does not affect the public API, so I can fix that without a breaking change.

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.

2 participants