Merged
Conversation
|
Size Change: -48 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
Mamaduka
added
[Type] Enhancement
walbo
reviewed
Aug 3, 2022
Member
walbo
left a comment
walbo
left a comment
There was a problem hiding this comment.
Good job so far! Left a few comments.
Looks like you also need to rebase the PR to get the changelog entry to the newest Unreleased section.
walbo
reviewed
Aug 8, 2022
ciampo
reviewed
Aug 8, 2022
Member
Author
|
Thanks for the review. I will update Unit test to ts as well. Will ping you when done! |
torounit
force-pushed
the
refactoring/components/disabled
branch
from
3b0e5fc to
3bfe8c9
Compare
torounit
commented
Aug 18, 2022
torounit
commented
Aug 18, 2022
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Petter Walbø Johnsgård <petter@dekode.no>
Co-authored-by: Petter Walbø Johnsgård <petter@dekode.no>
ciampo
reviewed
Aug 19, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Thank you @torounit for addressing the feedback so far!
I've left a few minor comments, but we should be quite close to being able to merge this PR!
One thing that I noticed during my tests, is that changing isDisabled from false to true will completely erase the value of the form elements:
Kapture.2022-08-19.at.13.21.43.mp4
This is not a blocker for this PR, since the same behavior can be observed on trunk — but at the same time, I'm not sure if we caught a bug or if the component was always supposed to work like this (cc @mirka). @gziolo , I see you had participate into some of work on this component (#20514 and #20766), do you remember if this behavior has always been around ?
| // | ||
| // https://github.com/jsdom/jsdom/issues/639 | ||
|
|
||
| describe( 'Consumer', () => { |
Contributor
There was a problem hiding this comment.
We could add a similar list of tests that consume the Content via useContent instead than using Disabled.Consumer — but that's definitely a task for a separate PR!
Member
Contributor
Alright, let's treat it as a bug — we'll try to trace when it was introduced and work on a fix + unit tests. |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
… into refactoring/components/disabled # Conflicts: # packages/components/src/disabled/test/index.tsx
Member
Author
Contributor
The approach looks promising, but let's work on it in a separate PR to keep this one focused on the TypeScript migration. Would you be able to remove 28820f0 from this PR and open a new PR with that fix instead? Once that's done and CHANGELOG conflicts are solved, we should be able to merge this one 🚀 |
ciampo
reviewed
Aug 22, 2022
Member
Author
ciampo
reviewed
Aug 23, 2022
ciampo
reviewed
Aug 23, 2022
ciampo
approved these changes
Aug 23, 2022
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #35744
What?
Converts the
Disabledcomponent to TypeScript.Testing Instructions
npm run storybook:devDisabledScreenshots or screencast