Skip to content

Conversation

@Shiva903-hue
Copy link

What does this PR do?

Preserves the default react-tabs className in UncontrolledTabs when
props.className is undefined or null.

Why?

Currently, spreading { ...defaultProps, ...props } causes an explicit
undefined or null className to override the default, which breaks
backward compatibility and styling.

Maintainers confirmed this behavior in #581.

What’s included

  • Guard against undefined / null overrides
  • Regression tests covering both cases

Related issue

Fixes #581

Copy link
Collaborator

@danez danez left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I left some comments.

Comment on lines +392 to +411
// Only pass valid DOM attributes and data- attributes
const domAttributes = {};
Object.keys(restProps).forEach((key) => {
if (
key.startsWith('data-') ||
key === 'id' ||
key === 'style' ||
key === 'title' ||
key === 'role' ||
key === 'tabIndex' ||
key === 'aria-label' ||
key === 'aria-labelledby' ||
key === 'aria-describedby' ||
key === 'aria-controls' ||
key === 'aria-selected' ||
key === 'aria-disabled'
) {
domAttributes[key] = restProps[key];
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This domAttributes is not needed. We remove all custom props above and pass on everything else.

Suggested change
// Only pass valid DOM attributes and data- attributes
const domAttributes = {};
Object.keys(restProps).forEach((key) => {
if (
key.startsWith('data-') ||
key === 'id' ||
key === 'style' ||
key === 'title' ||
key === 'role' ||
key === 'tabIndex' ||
key === 'aria-label' ||
key === 'aria-labelledby' ||
key === 'aria-describedby' ||
key === 'aria-controls' ||
key === 'aria-selected' ||
key === 'aria-disabled'
) {
domAttributes[key] = restProps[key];
}
});

return (
<div
{...attributes}
{...domAttributes}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{...domAttributes}
{...attributes}

environment, // unused here
disableUpDownKeys, // unused here
disableLeftRightKeys, // unused here
...restProps
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
...restProps
...attributes

Comment on lines +413 to +414
const className =
propClassName == null ? defaultProps.className : propClassName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const className =
propClassName == null ? defaultProps.className : propClassName;
const className = propClassName ?? defaultProps.className;

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.

className default is lost when props.className is undefined in UncontrolledTabs

2 participants