External menu configurations #289
Conversation
emmenko
added
🙏 Status: Dev Review
| }; | ||
| redirectTo = targetUrl => window.location.replace(targetUrl); | ||
| render() { | ||
| if (this.props.externalLink) { |
There was a problem hiding this comment.
This case was only used by the support link, which is now rendered separately. Hence the cleanup here
| @@ -317,23 +294,19 @@ export class DataMenu extends React.PureComponent { | |||
| data: PropTypes.arrayOf( | |||
| PropTypes.shape({ | |||
| key: PropTypes.string.isRequired, | |||
| labelKey: PropTypes.string, | |||
There was a problem hiding this comment.
All labels are now coming from labelAllLocales
| size="scale" | ||
| theme={getIconTheme( | ||
| isActive || this.isMainMenuRouteActive(menu.uriPath), | ||
| menu.shouldRenderDivider |
There was a problem hiding this comment.
This determines the "theme" of the icon now
| } | ||
| onMouseLeave={ | ||
| this.state.isMenuOpen ? null : this.shouldCloseMenuFly | ||
| <MenuItem |
There was a problem hiding this comment.
Nothing much changed here, mostly indentation
| </RestrictedMenuItem> | ||
| ); | ||
| }; | ||
|
|
||
| renderSupportLink = () => { |
There was a problem hiding this comment.
This is now rendered separately
| {this.props.data | ||
| .filter(menu => !this.bottomMenuItems.indexOf(menu.key)) | ||
| .map((menu, index) => this.renderMenu(menu, 'fixed', index))} | ||
| <div className={styles['scrollable-menu']}> |
There was a problem hiding this comment.
No need for a react component here, keep it simple
| <div className={styles['scrollable-menu']}> | ||
| {this.props.data.map((menu, index) => ( | ||
| <React.Fragment key={index}> | ||
| {menu.shouldRenderDivider && <MenuItemDivider />} |
There was a problem hiding this comment.
The divider is now rendered based on the flag shouldRenderDivider
| }; | ||
|
|
||
| export const NavBarLayout = props => ( | ||
| export const NavBarLayout = React.forwardRef((props, ref) => ( |
There was a problem hiding this comment.
NOTE that we use React.forwardRef, as documented here: https://reactjs.org/docs/forwarding-refs.html
| getNode = node => { | ||
| this.node = node; | ||
| }; | ||
| ref = React.createRef(); |
| : defaultNavigationItems | ||
| } | ||
| rootNode={this.ref.current} | ||
| data={navbarMenu.concat(customAppsMenu)} |
There was a problem hiding this comment.
Here we merge the internal apps with the custom apps config
| LoadingNavBar.displayName = 'LoadingNavBar'; | ||
| export class LoadingNavBar extends React.Component { | ||
| static displayName = 'LoadingNavBar'; | ||
| ref = React.createRef(); |
There was a problem hiding this comment.
Even though we don't use it here, the NavBarLayout expects a ref
| import React from 'react'; | ||
| import { shallow } from 'enzyme'; |
There was a problem hiding this comment.
FYI: when we re-implement the navbar, we can drop these tests and write RTL tests.
| email: PropTypes.string, | ||
| gravatarHash: PropTypes.string.isRequired, | ||
| }; | ||
| const UserSettingsMenuBody = props => { |
There was a problem hiding this comment.
This component is basically rendered only when the user opens the menu, therefore we can connect to fetch the menu config on this component, hence the split of components
| </div> | ||
| </Spacings.Inline> | ||
| </Spacings.Inset> | ||
| {menuLinks.map(menu => ( |
There was a problem hiding this comment.
This is basically the only thing that changed
| const noop = () => Component => Component; | ||
|
|
||
| const devonlyMenuLoader = | ||
| process.env.NODE_ENV !== 'development' |
There was a problem hiding this comment.
Dealing with loading the menu.json config for development was/is tricky to do, not much for the implementation but for the approach.
For instance, on production the application uses the https://mc.commercetools.com/api/graphql endpoint to load the configs through the Proxy server. In development though, there is no proxy running.
Furthermore, you want to load the menu.json of the application you're developing from.
In fact, you can't use the links to the other apps because they lead to 404 pages.
With that in mind, we need to disable the graphql queries to the proxy endpoint, since the URL is defined as ${window.location.origin}/api/graphql, which in development mode points to http://localhost:3001/api/graphql.
I thought about having a fake graphql endpoint in the webpack dev server, but there are some disadvantages with that:
- it's tight to webpack (e.g. if someone wants to develop an app using something different, there is a problem)
- we have duplication of the graphql schema in two places (app-kit + mc proxy)
I also tried using a custom apollo link to write to the cache with based on the menu.json, so that no network request is made since apollo will try to read from the cache. However, besides the implementation being a bit complex, it seems that apollo makes the request anyway, probably because the cache is not being filled in time.
This leaves us with the final approach I took, which is maybe not the ideal solution but it's good enough. One of the important things to keep in mind was that the setup should be as simple as possible for the user:
DEV_ONLY__getNavbarMenuConfig={() => import('../../../menu.json')}I chose to explicitly mark the prop with DEV_ONLY_ to make it clear that this won't be used in production.
Little downside: I suppose webpack will create a chunk for the menu.json, since it finds import() split point. We could e.g. wrap the call with a process.env.NODE_ENV so that uglily will remove it, but I'm not sure if that makes things simpler.
Please let me know what do you think about this approach, about naming etc. I'm open to suggestions 😇
There was a problem hiding this comment.
yeah maybe wrapping the call would be a good idea? but another unused chunk in prod doesn't really make any difference 🤔
There was a problem hiding this comment.
Could we also just have a HoC which reads the process.env to alternate the fetching behaviour? So instead of having a withDevOnlyMenuLoader and some graphql (Apollo) application we have a withMenuQuery and it alternates between fetching from the .json and through Apollo based on the env or a specific prop?
There was a problem hiding this comment.
Yeah, I think @montezume and I mean the same. Wrap the call in the HoC itself.
| const noop = () => Component => Component; | ||
|
|
||
| const devonlyMenuLoader = | ||
| process.env.NODE_ENV !== 'development' |
There was a problem hiding this comment.
yeah maybe wrapping the call would be a good idea? but another unused chunk in prod doesn't really make any difference 🤔
| @@ -40,6 +40,7 @@ | |||
| "LoginForm.forgotPassword": "Forgot password?", | |||
| "LoginForm.forgotPasswordTitle": "We are redirecting you to the Forgot Password form in {countdown} seconds.", | |||
| "LoginForm.InvalidCredentials": "Sorry, but the login details entered below are not in our system. Please enter your correct email address and password below.", | |||
| "LoginForm.invalidEmail": "Please enter a valid Email address.", | |||
There was a problem hiding this comment.
one day we will have translations that don't have German capitalization rules in English 😆
| const loadApplicationMessagesForLanguage = async lang => { | ||
| const messages = await import(`../../i18n/data/${lang}.json` /* webpackChunkName: "application-messages-[request]" */); | ||
| return messages.default; | ||
| }; |
There was a problem hiding this comment.
Unrelated refactoring, correct?
| ? noop | ||
| : (getLoaderFn, mapMenuToProps) => Component => { | ||
| class WrappedComponent extends React.Component { | ||
| static displayName = wrapDisplayName( |
There was a problem hiding this comment.
Nitpick: maybe not use recompose here as it's almost deprecated and we could aim to get rid of it in the app-shell maybe?
There was a problem hiding this comment.
I didn’t know that. What is the alternative then? We usually use it for compose
There was a problem hiding this comment.
Sorry, that came across wrong. I didn't mean to say "let's drop it" or that we decided. It was more of a suggestion: could the app-shell work without and if so what would be have to remove or custom build. Keep it for now. Should be a separate thing anyway. Sorry for the confusion.
There was a problem hiding this comment.
No worries, I understand your point. I was just asking what should we use to compose. Should we just copy the implementation and have it as an internal function?
There was a problem hiding this comment.
flowRight from lodash I suppose (the renamed compose to flowRight).
- I would have these HoCs as little utils, yes
- For the mc-fe it's almost unrealistic and not worthwhile getting rid of recompose
There was a problem hiding this comment.
Ah ok, good to know. We can do that as a follow up, I'll write it down on my list.
There was a problem hiding this comment.
packages/application-shell/src/components/user-settings-menu/user-settings-menu.js
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/navbar/devonly-menu-loader.js
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/user-settings-menu/user-settings-menu.js
Outdated
Show resolved
Hide resolved
| const noop = () => Component => Component; | ||
|
|
||
| const devonlyMenuLoader = | ||
| process.env.NODE_ENV !== 'development' |
There was a problem hiding this comment.
Could we also just have a HoC which reads the process.env to alternate the fetching behaviour? So instead of having a withDevOnlyMenuLoader and some graphql (Apollo) application we have a withMenuQuery and it alternates between fetching from the .json and through Apollo based on the env or a specific prop?
|
Alright, I've refactored the HOC based on the feedback: bbe6565
|
…ally loads local/remote menu config
emmenko
force-pushed
the
nm-external-menu
branch
from
326c445 to
bbe6565
Compare
| uri: `${environment.mcProxyApiUrl || | ||
| defaultApiUrl}/api/graphql`, | ||
| }} | ||
| onError={props.handleActionError} |
There was a problem hiding this comment.
FYI: this is not necessary anymore when we use #292
|
I think this is good to go now 🚀 PS: I'm not sure if this is considered a breaking change or just a feature 🤔 |
| response.setHeader('Content-Type', 'application/json'); | ||
| const errorMessage = `This GraphQL endpoint is not available in ${ | ||
| process.env.NODE_ENV | ||
| } mode as it's not necessary. The menu configuration is loaded from the file "menu.json" (more info at https://www.npmjs.com/package/@commercetools-frontend/application-shell). In case you do need to test things out, you can pass a "mcProxyApiUrl" to your "env.json" and point it to e.g. "https://mc.commercetools.com/api/graphql"`; |
packages/application-shell/src/components/with-applications-menu/with-applications-menu.spec.js
Outdated
Show resolved
Hide resolved
…rs (#292) * feat: add handleApolloErrors HOC to automatically dispatch query errors * refactor(with-applications-menu): remove error handling, fix props spreading * chore(handle-apollo-errors): add prop-types shape * refactor(handle-apollo-errors): renaming Co-Authored-By: emmenko <nicola.molinari@commercetools.de> * chore: format
Closes #37
The menu configuration for each application has been moved outside of the AppShell and is available as a GraphQL endpoint on the MC domain (not the MC API).
Each internal application (managed by CT) can now define their own menu config, which is then uploaded to a storage service so that the GraphQL server can aggregate those configs are expose them.
Custom applications still need to manage their settings separately, however they don't need to update the app-kit dependencies every time we change the menu links. 🎉
For the "account" links, it works the same way.
TODOS: