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 Categories selector for dynamic zone #7987

Merged
merged 13 commits into from
Nov 18, 2020
Merged

Added Categories selector for dynamic zone #7987

merged 13 commits into from
Nov 18, 2020

Conversation

misterdju
Copy link
Contributor

This RP corresponds to the development of the RFC (0019-dz-categories)
Hope this PR is ready to be merged :)

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #7987 (7dadfcc) into master (c1b7ab3) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7987      +/-   ##
==========================================
- Coverage   33.73%   33.65%   -0.09%     
==========================================
  Files        1244     1248       +4     
  Lines       13732    13765      +33     
  Branches     1365     1365              
==========================================
  Hits         4632     4632              
- Misses       8210     8243      +33     
  Partials      890      890              
Flag Coverage Δ
front 25.33% <0.00%> (-0.09%) ⬇️
unit 54.36% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...min/src/components/DynamicComponentCard/Wrapper.js 0.00% <ø> (ø)
...manager/admin/src/components/DynamicZone/Banner.js 0.00% <0.00%> (ø)
.../admin/src/components/DynamicZone/BannerWrapper.js 0.00% <0.00%> (ø)
...r/admin/src/components/DynamicZone/CategoryItem.js 0.00% <0.00%> (ø)
...admin/src/components/DynamicZone/ComponentsList.js 0.00% <0.00%> (ø)
...min/src/components/DynamicZone/ComponentsPicker.js 0.00% <ø> (ø)
...-manager/admin/src/components/DynamicZone/index.js 0.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7156c9...553a188. Read the comment docs.

@alexandrebodin
Copy link
Member

Hi your PR contains unwanted commits. Can you clean it up ?

Copy link
Contributor

@soupette soupette left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution!

Users are going to love this addition, there just a few changes that needs to be applied and we will merge your PR.

Don't hesitate if you have any question.

`
max-height: 260px;
`}

.componentPickerTitle {
margin-bottom: 15px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the margin-bottom to 10px in order to keep the baseline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soupette done in 7c6485e

<p className="componentPickerTitle">
<FormattedMessage id={`${pluginId}.components.DynamicZone.pick-compo`} />
</p>
<div className="categoriesList">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the following css so the baseline is kept

.categoriesList {
  padding-bottom: 4px;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soupette done in 8ff2eac

@@ -41,19 +42,27 @@ const DynamicZone = ({
dynamicDisplayedComponents,
}) => {
const [isOpen, setIsOpen] = useState(false);

const [indexCategory, setIndexCategory] = useState(-1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to change the logic a bit of the collapses.

So here's my idea, we will use an object instead for the dynamicComponentCategories.

First let's change the useState hook:

const [categoryToOpen, setCategoryToOpen] = useState('');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soupette done in 15570e0

@@ -78,6 +87,42 @@ const DynamicZone = ({
[layout, name]
);

const dynamicComponentCategories = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we will use lodash groupBy instead to simplify the code a bit:

const dynamicComponentCategories = useMemo(() => {
  const componentsWithInfos = dynamicZoneAvailableComponents.map(componentUid => {
    const {
      category,
      schema: { info },
    } = getDynamicComponent(componentUid);

    return { componentUid, category, info };
  });

  return groupBy(componentsWithInfos, 'category');
}, [dynamicZoneAvailableComponents, getDynamicComponent]);

This will return an object like the following:

{
  category1: [ { componentUid, category: category1: info }, ....],
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soupette done in 15570e0

}, []);
}, [dynamicZoneAvailableComponents, getDynamicComponent]);

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

With the modification added the effect is not needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soupette done in 15570e0

In the case there is only one category the list will not be opened by default.

<FormattedMessage id={`${pluginId}.components.DynamicZone.pick-compo`} />
</p>
<div className="categoriesList">
{dynamicComponentCategories.map(({ category, components }, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Therefore, this needs to be updated as follow:

{Object.keys(dynamicComponentCategories).map((categoryName, index) => {
  const components = dynamicComponentCategories[categoryName];

  return (
    <CategoryItem
      key={categoryName}
      category={categoryName}
      components={components}
      isOpen={categoryToOpen === categoryName}
      isFirst={index === 0}
      onClickToggle={() => {
       // Detailed below
        handleClickToggle(categoryName);
      }}
      onClickComponent={componentUid => {
        setCategoryToOpen('');
        const shouldCheckErrors = hasError;
        addComponentToDynamicZone(name, componentUid, shouldCheckErrors);
      }}
    />
  );
})}
const handleClickToggle = useCallback(
  categoryName => {
    const nextCategoryToOpen = categoryToOpen === categoryName ? '' : categoryName;

    setCategoryToOpen(nextCategoryToOpen);
  },
  [categoryToOpen]
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soupette done in 15570e0

align-self: center;
margin-top: -2px;
}
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello 😃 , I made some modifications to the file in order to use the theme and avoid the multiple conditions on the isOpen variable.

import styled from 'styled-components';

/* eslint-disable */

const BannerWrapper = styled.button`
  display: flex;
  height: 36px;
  width: 100%;
  padding: 0 15px;
  border-bottom: 0;
  border: 1px solid rgba(227, 233, 243, 0.75);
  background-color: ${({ theme }) => theme.main.colors.white};
  font-size: ${({ theme }) => theme.main.sizes.fonts.md};
  font-weight: ${({ theme }) => theme.main.fontWeights.semiBold};
  cursor: pointer;

  &:focus {
    outline: 0;
  }

  .img-wrapper {
    width: 19px;
    height: 19px;
    margin-right: 19px;
    border-radius: 50%;
    background-color: ${({ theme }) => theme.main.colors.mediumGrey};
    text-align: center;
  }
  .label {
    text-transform: capitalize;
  }

  svg {
    path {
      fill: ${({ theme }) => theme.main.colors.leftMenu['link-color']} !important;
    }
  }

  -webkit-font-smoothing: antialiased;

  > div {
    align-self: center;
    margin-top: -2px;
  }

  ${({ isFirst, theme }) => {
    if (isFirst) {
      return `
        border-top-right-radius: ${theme.main.sizes.borderRadius};
        border-top-left-radius: ${theme.main.sizes.borderRadius};
      `;
    }
  }}

  ${({ isOpen, theme }) => {
    if (isOpen) {
      return `
        border: 1px solid ${theme.main.colors.darkBlue};
        background-color: ${theme.main.colors.lightBlue};
        color: ${theme.main.colors.mediumBlue};
        font-weight: ${theme.main.fontWeights.bold};

        .img-wrapper {
          background-color: ${theme.main.colors.darkBlue};
          transform: rotate(180deg);
        }

        svg {
        path {
            fill: ${theme.main.colors.mediumBlue} !important;
          }
        }
      `;
    }
  }}
`;

export default BannerWrapper;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<Collapse
isOpen={isOpen}
style={{ backgroundColor: '#F3F3F3' }}
onExited={() => setShowComponents(false)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a handle function please.

const handleExited = () => setShowComponents(false);

return (
    <>
      <Banner onClickToggle={onClickToggle} isFirst={isFirst} isOpen={isOpen} category={category} />
      <Collapse
        isOpen={isOpen}
        style={{ backgroundColor: '#F3F3F3' }}
        onExited={handleExited}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<Banner onClickToggle={onClickToggle} isFirst={isFirst} isOpen={isOpen} category={category} />
<Collapse
isOpen={isOpen}
style={{ backgroundColor: '#F3F3F3' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This custom background seems to be useless. Can you remove it please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<div
className="componentsList"
style={{ paddingTop: '10px', paddingLeft: '15px', paddingRight: '15px' }}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a file for this component please.

`...src/components/DynamicZone/ComponentsList.js``

import styled from 'styled-components';

const ComponentsList = styled.div`
  padding-top: 10px;
  padding-left: 15px;
  padding-right: 15px;
`;

export default ComponentsList

and then in .../src/components/DynamicZone/CategoryItem.js

...
return (
    <>
      <Banner onClickToggle={onClickToggle} isFirst={isFirst} isOpen={isOpen} category={category} />
      <Collapse isOpen={isOpen} onExited={handleExited}>
        {showComponents && (
          <ComponentsList className="componentsList">
            {components.map(component => {
              const {
                info: { icon, name: friendlyName },
                componentUid,
              } = component;

              return (
                <DynamicComponentCard
                  key={componentUid}
                  componentUid={componentUid}
                  friendlyName={friendlyName}
                  icon={icon}
                  onClick={() => onClickComponent(componentUid)}
                />
              );
            })}
          </ComponentsList>
        )}
      </Collapse>
    </>
  );
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@misterdju misterdju requested a review from a team as a code owner October 26, 2020 15:14
@misterdju misterdju requested review from soupette and HichamELBSI and removed request for a team October 28, 2020 17:17
return groupBy(componentsWithInfos, 'category');
}, [dynamicZoneAvailableComponents, getDynamicComponent]);

// useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the comment?

soupette
soupette previously approved these changes Nov 2, 2020
Copy link
Contributor

@soupette soupette left a comment

Choose a reason for hiding this comment

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

LGTM

@soupette soupette added the source: core:content-manager Source is core/content-manager package label Nov 2, 2020
@soupette soupette modified the milestone: 3.2.6 Nov 2, 2020
@soupette
Copy link
Contributor

soupette commented Nov 3, 2020

@misterdju can you make sure the DCO checks so we can merge your PR?

Boudringhin Julien and others added 3 commits November 3, 2020 14:53
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
Signed-off-by: Nicolas Chesné <hello@norbz.net>
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
@misterdju
Copy link
Contributor Author

@misterdju can you make sure the DCO checks so we can merge your PR?

@soupette, I have done the commands suggested by the DCO checks. Is it ok for you ?

@soupette
Copy link
Contributor

@misterdju sure but it looks like you have unwanted commits in your PR which affects more than 100 files. Can you check?

@derrickmehaffy
Copy link
Member

I think a rebase here instead of merging in the master should remove quite a bit of the conflicts as well as the various commits

jbo@lachose.fr added 3 commits November 16, 2020 12:12
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
jbo@lachose.fr and others added 6 commits November 16, 2020 12:14
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
Signed-off-by: Julien Boudringhin <julien.boudringhin@gmail.com>
@misterdju
Copy link
Contributor Author

@soupette i think it is good now.

Copy link
Contributor

@soupette soupette left a comment

Choose a reason for hiding this comment

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

LGTM

@soupette soupette added this to the 3.4.0 milestone Nov 18, 2020
@soupette
Copy link
Contributor

@misterdju can you update your branch so we can merge the PR?

@soupette soupette merged commit 3cb5396 into strapi:master Nov 18, 2020
@soupette soupette added the issue: enhancement Issue suggesting an enhancement to an existing feature label Nov 18, 2020
@alexandrebodin alexandrebodin modified the milestones: 3.4.0, 3.3.4 Nov 25, 2020
@derrickmehaffy
Copy link
Member

This pull request has been mentioned on Strapi Community. There might be relevant details there:

https://forum.strapi.io/t/new-release-strapi-v3-3-4/1322/1

@misterdju misterdju deleted the feature/dynamiczone-categories branch November 26, 2020 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: enhancement Issue suggesting an enhancement to an existing feature source: core:content-manager Source is core/content-manager package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants