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

Category list component #171

Merged
merged 5 commits into from Jul 30, 2018
Merged

Conversation

artKozinets
Copy link
Contributor

@artKozinets artKozinets commented Jul 25, 2018

This PR is a:

[X] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

New root Category list component to show subcategories from parent category.

Issue

https://app.zenhub.com/workspace/o/magento-research/pwa-studio/issues/99

Additional information

How to use

Add to the page <CategoryList></CategoryList>

Options

title - Category list component title, appears at the top of the block.
id - Main category id, to show child categories from.

Example

<CategoryList title={'Shop by category'}></CategoryList>

<CategoryList title={'Shop by category'} id={3}></CategoryList>

@coveralls
Copy link

coveralls commented Jul 25, 2018

Pull Request Test Coverage Report for Build 316

  • 0 of 21 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 55.556%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/venia-concept/src/RootComponents/CMS/CMS.js 0 3 0.0%
packages/venia-concept/src/components/CategoryList/categoryList.js 0 18 0.0%
Totals Coverage Status
Change from base Build 291: -0.03%
Covered Lines: 809
Relevant Lines: 1506

💛 - Coveralls

Copy link
Contributor

@zetlen zetlen left a comment

This is great implementation and it works wonderfully. Thanks for contributing it. We do need a couple of changes; most urgently, we need to remove hardcoded base URLs. Please ask us if you need any help refactoring those out.

return <div>CMS Page Stub</div>;
return (
<Page>
<CategoryList title={'Shop by category'} id={2} />
Copy link
Contributor

@zetlen zetlen Jul 26, 2018

Choose a reason for hiding this comment

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

This is a small thing, but JSX allows you to use literal strings as attribute values without the expression braces. You have to use double quotes, so this could be:

<CategoryList title="Shop by category" id={2} />

Soon we will create an internationalization system that finds and replaces these strings, so they should use a common syntax.

.content {
display: grid;
grid-gap: 3rem 1rem;
grid-template-columns: repeat(auto-fit, 6rem);
Copy link
Contributor

@zetlen zetlen Jul 26, 2018

Choose a reason for hiding this comment

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

@jimbo would give better advice here, but I think you might want to use a fr unit instead of 6rem, if you want a predictable number of items in each row.

Copy link
Contributor Author

@artKozinets artKozinets Jul 26, 2018

Choose a reason for hiding this comment

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

We have mobile, tablet, desktop view.
My idea was to have fixed width of items + margin between them.
Center aligned, no matter which device we are talking about.

screen shot 2018-07-26 at 17 39 51
screen shot 2018-07-26 at 17 40 23

If you have some other solution please let me know.

Copy link
Contributor

@jimbo jimbo Jul 30, 2018

Choose a reason for hiding this comment

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

The approach used here achieves the design they're aiming for, so it looks good to me. Thanks for embracing grid. 👍

import classify from 'src/classify';
import defaultClasses from './categoryList.css';

// TODO: get only active categories from graphql when it is ready
Copy link
Contributor

@zetlen zetlen Jul 26, 2018

Choose a reason for hiding this comment

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

Thank you for adding this TODO comment. This really helps everyone. 👍

`;

// TODO: get baseUrl from graphql when it is ready
const baseUrl = 'https://magento-venia.local.pwadev:8000';
Copy link
Contributor

@zetlen zetlen Jul 26, 2018

Choose a reason for hiding this comment

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

You should not need to hardcode the full domain! Relative paths from root should be fine. Are you working with Luma sample data?

Copy link
Contributor Author

@artKozinets artKozinets Jul 26, 2018

Choose a reason for hiding this comment

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

Yep, I use magento 2.3-develop + sampledata

const baseUrl = 'https://magento-venia.local.pwadev:8000';

// TODO: get mediaUrl from graphql when it is ready
const mediaUrl = 'https://magento-venia.local.pwadev:8000/pub/media';
Copy link
Contributor

@zetlen zetlen Jul 26, 2018

Choose a reason for hiding this comment

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

Same here; this variable should probably be pub/media.

item: string,
imageWrapper: string,
image: string,
name: string
Copy link
Contributor

@zetlen zetlen Jul 26, 2018

Choose a reason for hiding this comment

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

Excellent work implementing a public CSS classes API.

};

static defaultProps = {
id: 2
Copy link
Contributor

@zetlen zetlen Jul 26, 2018

Choose a reason for hiding this comment

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

This component probably shouldn't have a default ID. I'm guessing you made this for easy testing, but it should be removed.

Copy link
Contributor Author

@artKozinets artKozinets Jul 26, 2018

Choose a reason for hiding this comment

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

Idea was to have preset of the default magento category.
"Root Catalog" - id="1"
"Default Category" - id="2"


return (
<div className={classes.root}>
{this.props.title && (
Copy link
Contributor

@zetlen zetlen Jul 26, 2018

Choose a reason for hiding this comment

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

We have developed a pattern for nullable props that should conditionally render a portion of JSX. (@jimbo and I should probably document this pattern). This could be another component, but it seems too single-purpose; our pattern is to create a class getter that encapsulates this logic. An example here shows a property {this.options} that you can simply embed in your JSX with no conditional, because the conditional is implemented in the getter.

Here, you should add a class method:

get header() {
  const { title, classes} = this.props;
  return title ? (
    <div className={classes.header}>
        <h2 className={classes.title}>
            <span>{title}</span>
        </h2>
    </div>
  ) : null;
}

And then in your render method right here, you replace this expression with

<div className={classes.root>
  {this.header}
  <Query query={categoryListQuery} variables= {{ id }}>

(We recommend a getter instead of a plain accessor function, because a getter prevents you from passing arguments internally, which could put logic where it doesn't belong. A getter ensures that the method must have the same signature as render(), so that it could easily be moved into another component if necessary.)


return (
<div className={classes.content}>
{data.category.children.map((item, index) => (
Copy link
Contributor

@zetlen zetlen Jul 26, 2018

Choose a reason for hiding this comment

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

How would you feel about implementing the inner function of .map here as another Component instead of inline JSX? It's not mandatory, but it's something to think about.

Copy link
Contributor Author

@artKozinets artKozinets Jul 26, 2018

Choose a reason for hiding this comment

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

Yep, awesome idea.

Copy link
Contributor

@jimbo jimbo Jul 30, 2018

Choose a reason for hiding this comment

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

I know we've merged this already, but yes—in general we try to avoid performing work inside JSX expressions. Eventually, I'd like to cover that with a lint rule. Certainly not a dealbreaker, though, so thanks for your understanding. 😄

@magento-cicd2
Copy link

magento-cicd2 commented Jul 27, 2018

CLA assistant check
All committers have signed the CLA.

zetlen
zetlen approved these changes Jul 30, 2018
@zetlen
Copy link
Contributor

zetlen commented Jul 30, 2018

@artKozinets Changes look great and I'm ready to merge. Can you please sign the Contributor License Agreement posted above?

@artKozinets
Copy link
Contributor Author

artKozinets commented Jul 30, 2018

@zetlen Done.

@zetlen zetlen merged commit 332cb41 into magento:master Jul 30, 2018
2 checks passed
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

5 participants