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

Added a not found view to product grid #2656

Merged
merged 15 commits into from
Aug 11, 2017
Merged

Conversation

mikemurray
Copy link
Member

  • Added new component NotFound that lets you do blank views
  • Added not found view to product grid if tag was not found

- Added new component NotFound that lets you do blank views
- Added not found view to product grid if tag was not found
@mikemurray mikemurray mentioned this pull request Aug 8, 2017
@mikemurray mikemurray changed the title [WIP] Added a not found view to product grid Added a not found view to product grid Aug 9, 2017
@brent-hoover
Copy link
Collaborator

@mikemurray If I go to a non-existent tag I don't get a 404 I get a "No Products Found". Is that the correct behavior?

@brent-hoover
Copy link
Collaborator

Is it my imagination or does rendering of products now seem considerably slower?

@brent-hoover
Copy link
Collaborator

If I go to a tag with no products I just get nothing rather than the "No Products Found" page.

@mikemurray
Copy link
Member Author

I think I only did, if the tag does not exist, then no products found; which is more like a tag not found. Should extend this to, if the products array is empty, then no product found as well.


The load may be slower / feel slower.

@brent-hoover
Copy link
Collaborator

I feel like if I go to a tag with no products I should get the "No product" message rather than just a blank page which looks broken. What do you think?

That's the way it currently works on master (blank page) so it's not a bug/blocker

@mikemurray
Copy link
Member Author

I agree, would be easy to add I think.

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Just typos in comments

}, props.className);

// ClassName for content wrapper
// If one is provied in props, the default is not used
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo in comment


/**
* Baase wrapper className
* @summary String className of `classnames` compatable object for the base wrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo in comment

NotFound.propTypes = {
/**
* Children
* @type {ReactNode}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make this more consistent? I feel like we call this different things different place (e.g. Node/ReactNode/JSX. Is there a difference?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe just go with Node prop type.

@machikoyasuda what do you think? Maybe we can set up a type in jsdoc that knows Node is react node when the docs are generated?

children: PropTypes.node,

/**
* Baase wrapper className
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo in comment


/**
* Content className
* @summary String className of `classnames` compatable object for the content wrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo in comment (compatible)

@mikemurray
Copy link
Member Author

@zenweasel Should now show the not-found view if the tag is valid, but no products were found still.

@machikoyasuda As a bonus, jsdoc'd all the things in Products component.


/**
* Base wrapper className
* @summary String className or `classnames` compatable object for the base wrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

compatible still spelled wrong isn't it?

@brent-hoover
Copy link
Collaborator

@mikemurray three lint errors that seem to come from nowhere

@mikemurray
Copy link
Member Author

@zenweasel wonder if something changed in our eslint config since those files were passing before, right?

@mikemurray
Copy link
Member Author

Fixed all lint issues

@brent-hoover
Copy link
Collaborator

@mikemurray yeah, I don't know where those lint errors came from but thanks for fixing them

Copy link
Contributor

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

Approve - I added a commit to change the PropType JSDoc style like we talked about IRL. This format will actually render it into the docs.

@brent-hoover
Copy link
Collaborator

@mikemurray @machikoyasuda still two lint errors left

@mikemurray
Copy link
Member Author

mikemurray commented Aug 11, 2017

ok, whats going on. I fixed all the lint errors and more keep popping up?

@mikemurray
Copy link
Member Author

All green!

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.

3 participants