Create separate provider for the context #1050
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: TimothyAsirJeyasing The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@SanjalKatiyar Please review |
| activeItem, | ||
| setActiveItem, | ||
| }), | ||
| [activeItemsUID, setActiveItemsUID, activeItem, setActiveItem] |
There was a problem hiding this comment.
Please resolve these dependency warnings.
There was a problem hiding this comment.
please do not resolve comments unless it is fixed... I can still see the same warnings...
TimothyAsirJeyasing
force-pushed
the
create-seperate-provider-for-context
branch
from
831ebe1
to
710ab54
Compare
|
build tests are failing |
|
@TimothyAsirJeyasing: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest |
TimothyAsirJeyasing
force-pushed
the
create-seperate-provider-for-context
branch
from
710ab54
to
4b944ef
Compare
| @@ -531,24 +531,53 @@ const Topology: React.FC = () => { | |||
|
|
|||
| const zones = memoizedNodes.map(getTopologyDomain); | |||
|
|
|||
| const TopologyDataContextProvider: React.FC<{ children: React.ReactNode }> = | |||
There was a problem hiding this comment.
why are you creating nested FCs ?
There was a problem hiding this comment.
create sperate provider...
There was a problem hiding this comment.
Could you please give some more light
There was a problem hiding this comment.
do not create a FC inside another FC...
| ); | ||
| }; | ||
|
|
||
| const TopologySearchContextProvider: React.FC<{ children: React.ReactNode }> = |
There was a problem hiding this comment.
create sperate provider... do not do nesting of FCs...
|
is this PR tested ? |
TimothyAsirJeyasing
force-pushed
the
create-seperate-provider-for-context
branch
from
4b944ef
to
d5d30db
Compare
|
|
| export const combineComponents = (...components: React.FC[]): React.FC => { | ||
| return components.reduce( | ||
| (AccumulatedComponents, CurrentComponent) => { | ||
| return ({ children }: React.ComponentProps<React.FC>): JSX.Element => { | ||
| return ( | ||
| <AccumulatedComponents> | ||
| <CurrentComponent>{children}</CurrentComponent> | ||
| </AccumulatedComponents> | ||
| ); | ||
| }; | ||
| }, | ||
| ({ children }) => <>{children}</> | ||
| ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
no need for this...
|
|
||
| const providers = [CSVStatusesContextProvider, DRResourcesContextProvider]; | ||
|
|
||
| const AppContextProvider = combineComponents(...providers); |
There was a problem hiding this comment.
no need for this right now...
| // ToDo(Sanjal): combime multiple Context together to make it scalable | ||
| // refer: https://javascript.plainenglish.io/how-to-combine-context-providers-for-cleaner-react-code-9ed24f20225e |
There was a problem hiding this comment.
add this comment back...
…a childern Signed-off-by: Timothy Asir Jeyasingh <tjeyasin@redhat.com>
TimothyAsirJeyasing
force-pushed
the
create-seperate-provider-for-context
branch
from
d5d30db
to
6a1e820
Compare
|
@SanjalKatiyar Please review |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
After encountering many changes and merge conflicts, I am closing this and I will be sending a separate pull request. /close |
Create separate provider for the context and pass its descendants as a children
#672