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

Layout v2 component #1821

Merged
merged 13 commits into from
Jun 25, 2020
Merged

Layout v2 component #1821

merged 13 commits into from
Jun 25, 2020

Conversation

andresmgot
Copy link
Contributor

@andresmgot andresmgot commented Jun 24, 2020

Description of the change

The Layout is the first self-written component that we render. I am following the same approach than in #1814 to conditionally load one file or another depending on the UI in the configuration. Note that the requisites to do this are:

  • The component should have a container. This is needed to pass the featureFlags as a property. Most of our components already have this but it was not the case for the Layout component.
  • The index file of the component will decide which of the two versions it should load.
  • A second file (I've chosen to use the notation .v2) should be loaded for the Clarity UI.

Result:

With the hex UI:

Screenshot from 2020-06-24 12-27-20

With the clarity UI:

Screenshot from 2020-06-24 12-26-49

Note that the Layout component is just a wrapper, we still need to work in all the components of the different views.

Applicable issues

Additional information

The change of base branch messed a bit the git history but I've also moved the UISelector to the layout component, I believe there is a better place rather than in the Header component.

Base automatically changed from uiSelector to master June 24, 2020 10:56
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

See inline comments. I think it'd be worth waiting for the package updates so we no longer need to wrap components in containers to access state.

};
}

export default connect(mapStateToProps)(Layout);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: you might to wait or my branch updating our redux/react-router dependencies etc. as you'll no longer need a container to access state (as it can be accessed via hooks). Similarly, updating connected-react-router forces adding the router state to the IStoreState as you've done manually here (ie. it'll already be done in my branch).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to merge this before so I don't have conflicts since it's quite simple, in any case, the route thing is not needed.

@andresmgot andresmgot merged commit 9ae081d into master Jun 25, 2020
@andresmgot andresmgot deleted the layoutSelector branch June 25, 2020 15:20
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.

None yet

2 participants