RFC for converged implementation pattern#16806
Merged
ling1726 merged 35 commits intomicrosoft:masterfrom Feb 12, 2021
Merged
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0d05ded:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: fe4959d22fec3382fa743b396f671b1b63a26b9e (build) |
Collaborator
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
kubkon
reviewed
Feb 4, 2021
kubkon
reviewed
Feb 4, 2021
kubkon
reviewed
Feb 4, 2021
kubkon
reviewed
Feb 4, 2021
Co-authored-by: Jakub Konka <kubkon@jakubkonka.com>
kubkon
reviewed
Feb 4, 2021
Co-authored-by: Jakub Konka <kubkon@jakubkonka.com>
kubkon
reviewed
Feb 4, 2021
kubkon
reviewed
Feb 4, 2021
kubkon
reviewed
Feb 4, 2021
kubkon
reviewed
Feb 4, 2021
Co-authored-by: Jakub Konka <kubkon@jakubkonka.com>
kubkon
reviewed
Feb 4, 2021
kubkon
reviewed
Feb 4, 2021
Contributor
|
This is absolutely amazing, thanks so much @ling1726! All in all, it reads really well, and is simple to understand. I did leave a couple of nits/questions but otherwise, great job! |
Co-authored-by: Jakub Konka <kubkon@jakubkonka.com>
Co-authored-by: Jakub Konka <kubkon@jakubkonka.com>
Add more explanation for render func
Modified order
Clarified render func responsibility
layershifter
reviewed
Feb 8, 2021
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
layershifter
reviewed
Feb 8, 2021
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
layershifter
reviewed
Feb 8, 2021
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
layershifter
reviewed
Feb 8, 2021
layershifter
reviewed
Feb 8, 2021
Hotell
reviewed
Feb 8, 2021
|
|
||
| // merges the props we declare internally and what is passed in | ||
| // by a consumer | ||
| const mergedProps = mergeProps( |
Contributor
There was a problem hiding this comment.
suggestion: If we wanna provide intuitive API's I'd suggest to reflect that in naming.
eg, from consumer point of view:
// ah makes sense? I guess
- const mergedProps = mergeProps()
// what, merge props returns draft state ?
- const draftState = mergeProps()
// explicit api with good enough naming
+const {state} = mergeProps()| ); | ||
|
|
||
| if (checkedValues || onCheckedValuesChange) { | ||
| mergedProps.hasCheckMark = true; |
Contributor
There was a problem hiding this comment.
thought: ability to be able to directly mutate magical dictionary feels kinda wrong. Do we really need to provide apis like this ?
Contributor
Author
There was a problem hiding this comment.
I would love for immutable state, but this is AFAIK what all the current utils and tooling works on
| React.useEffect(...); | ||
|
|
||
| // make an 'uber' state | ||
| const state = { someState, contextValue, ...mergedProps} |
Contributor
There was a problem hiding this comment.
I'd love to agree with you, but from my experience you cannot expect anything unfortunately (unless it can be automated/enforced by tooling). Developers tend to always find a way how to do "workarounds" around expectations :D
use state in makestyles
Hotell
reviewed
Feb 8, 2021
Co-authored-by: Martin Hochel <hochelmartin@gmail.com>
layershifter
reviewed
Feb 8, 2021
layershifter
reviewed
Feb 8, 2021
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
joshualamusga1
pushed a commit
to joshualamusga1/fluentui
that referenced
this pull request
Feb 25, 2021
* RFC for converged implementation pattern * Update rfcs/converged-implementation-patterns.md Co-authored-by: Jakub Konka <kubkon@jakubkonka.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Jakub Konka <kubkon@jakubkonka.com> * Update converged-implementation-patterns.md * Update rfcs/converged-implementation-patterns.md Co-authored-by: Jakub Konka <kubkon@jakubkonka.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Jakub Konka <kubkon@jakubkonka.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Jakub Konka <kubkon@jakubkonka.com> * Update converged-implementation-patterns.md * Update converged-implementation-patterns.md Add more explanation for render func * Update converged-implementation-patterns.md * Update converged-implementation-patterns.md Modified order * Update converged-implementation-patterns.md Clarified render func responsibility * Update converged-implementation-patterns.md * Update converged-implementation-patterns.md fix typo * Update rfcs/converged-implementation-patterns.md Co-authored-by: Jakub Konka <kubkon@jakubkonka.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Jakub Konka <kubkon@jakubkonka.com> * Update converged-implementation-patterns.md * Update converged-implementation-patterns.md * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> * Update converged-implementation-patterns.md type slots * Update converged-implementation-patterns.md use state in makestyles * Update converged-implementation-patterns.md * Update converged-implementation-patterns.md * Update converged-implementation-patterns.md * Update rfcs/converged-implementation-patterns.md * Update rfcs/converged-implementation-patterns.md Co-authored-by: Martin Hochel <hochelmartin@gmail.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> * Update rfcs/converged-implementation-patterns.md Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> Co-authored-by: Jakub Konka <kubkon@jakubkonka.com> Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com> Co-authored-by: Martin Hochel <hochelmartin@gmail.com>
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
8 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.
Description of changes
This RFC attempts to document and find some quorum on converged component implementation patterns. This has been a pain point for newer engineers joining the team at a time when the patterns themselves might be undergoing change.
The best result (I can think of) would be to output the conclusion code samples in this RFC into automated package generation, and keep this RFC updated (I am hopeful....) if we continue our state of change on these issues