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

**Fix:** it was not possible to disable Tree component inside CardSection #953

Merged
merged 1 commit into from
Mar 7, 2019

Conversation

stereobooster
Copy link
Contributor

@stereobooster stereobooster commented Mar 7, 2019

Summary

screenshot 2019-03-07 at 13 45 45

Related issue

N/A

To be tested

Me

  • No error or warning in the console on localhost:6060

Tester 1

  • Things look good on the demo.

Tester 2

  • Things look good on the demo.

@stereobooster stereobooster self-assigned this Mar 7, 2019
@stereobooster stereobooster requested a review from TejasQ March 7, 2019 12:45
@@ -43,10 +43,11 @@ export type OverlayType = "noOverlay" | "disabled" | DragAndDropFeedback
* The flex rule only kicks in when the parent is flex-positioned in case
* sections are stacked horizontally.
*/
const Container = styled("div")`
const Container = styled("div")<{ disabled?: boolean }>`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we need to add aria-disabled

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but not here.

TejasQ
TejasQ previously approved these changes Mar 7, 2019
@@ -43,10 +43,11 @@ export type OverlayType = "noOverlay" | "disabled" | DragAndDropFeedback
* The flex rule only kicks in when the parent is flex-positioned in case
* sections are stacked horizontally.
*/
const Container = styled("div")`
const Container = styled("div")<{ disabled?: boolean }>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but not here.

@@ -161,7 +162,7 @@ const CardSection: React.SFC<CardSectionProps> = ({
forceToggleHoverStyles,
...props
}) => (
<Container {...props}>
<Container {...props} disabled={disabled}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Container {...props} disabled={disabled}>
<Container {...props} disabled={disabled} aria-disabled={disabled}>

@TejasQ TejasQ dismissed their stale review March 7, 2019 14:00

Wrong

Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

master

image

HEAD

image

Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

No regression

@TejasQ TejasQ merged commit 47953f4 into master Mar 7, 2019
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