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

feat: add landing page for catalyst #5226

Merged
merged 6 commits into from
Jun 24, 2019

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Jun 20, 2019

Impact: minor
Type: feature

Issue

When a user first lands on the Operator UI, a blank screen is shown, which makes it look broken.

Solution

Add a landing page that just lets the user know where they are, and provides links to docs, and common features.

Breaking changes

None

Testing

  1. Go to /operator
  2. See the landing page
  3. Click other links and make sure they all still work, and the the landing link doesn't override them.

Not-found

Signed-off-by: Erik Kieckhafer <[email protected]>
@kieckhafer kieckhafer changed the base branch from master to develop June 20, 2019 18:15
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
@kieckhafer kieckhafer changed the title [WIP] feat: add landing page for catalyst feat: add landing page for catalyst Jun 20, 2019
@kieckhafer
Copy link
Member Author

@rymorgan @cassytaylor just wanted to put something here so that there isn't a blank page when you first log into the Operator UI. I think this passes as a first draft of this, just to get it up there, but happy to discuss improvements if you think this needs to be changed before it's merged.

@kieckhafer kieckhafer requested a review from aldeed June 20, 2019 18:51
@kieckhafer
Copy link
Member Author

kieckhafer commented Jun 20, 2019

@aldeed could you please take a look specifically at how the Route is added to the Switch, just want to make sure I'm doing that in a legit way: https://github.com/reactioncommerce/reaction/pull/5226/files#diff-44c61a0d4d7989c935d5505b71eff89dR130

I was initially adding it using the registerOperatorRoute function, but it was getting loaded before other routes were added to that array, therefore overriding them so every page would just show this component.

We could add it to the very end of that array, knowing our current components that are loaded, however if someone adds a new route in the future that happens to get loaded after this one, then it would cause the same problem, which is why i did it this way.

@kieckhafer kieckhafer marked this pull request as ready for review June 20, 2019 18:53
@rymorgan
Copy link
Contributor

I'm good with this as a draft. I think in the near future we should leverage this page as an onboarding dashboard and possibly an alert center but let's give that some thought. Having it look not broken is a win.

Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

@kieckhafer I think if you add exact prop to the Route in the map (it's a boolean prop so just needs to be present with no value), then registering it will work. I'm guessing none of our other registered components are relying on the default partial path matching, but a quick skim should be enough to see if it broke any other routes.

https://reacttraining.com/react-router/web/api/Route/exact-bool

Signed-off-by: Erik Kieckhafer <[email protected]>
@kieckhafer
Copy link
Member Author

@aldeed I don't think any other components are relying on the default, I took a look and didn't' see anything. Plus the blank page... so if something was relying on it, it wasn't working anyway.

Adding the exact prop works, PR updated, ready for another look.

@kieckhafer kieckhafer requested review from aldeed and removed request for cassytaylor June 21, 2019 18:47
@aldeed
Copy link
Contributor

aldeed commented Jun 24, 2019

👍 Leaving for @machikoyasuda to merge if this looks good.

</Grid>
<Grid item>
<Typography align="center" variant="body2">
See our <MuiLink href="https://docs.reactioncommerce.com/docs/dashboard">Store Operator's Guide</MuiLink> for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
See our <MuiLink href="https://docs.reactioncommerce.com/docs/dashboard">Store Operator's Guide</MuiLink> for more information.
See our <MuiLink href="https://docs.reactioncommerce.com/docs/dashboard">Store Operators Guide</MuiLink> for more information.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kieckhafer Looks good! I just had this tiny tiny nitpick. If you want, I can commit this and merge it.

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries, I updated it, thanks for catching that! Should be ready now.

Signed-off-by: Erik Kieckhafer <[email protected]>
@machikoyasuda machikoyasuda merged commit 931dc4f into develop Jun 24, 2019
@machikoyasuda machikoyasuda deleted the feat-kieckhafer-addOperatorUiLandingComponent branch June 24, 2019 20:50
@jeffcorpuz jeffcorpuz mentioned this pull request Jul 2, 2019
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.

4 participants