Refactored theme components and added cypress test. #4843

Tishasoumya-02 · 2023-06-04T03:17:39Z

Refactored Components

Used useSelector,useDispatch,useEffect,useState hooks instead of lifecycle methods and redux functions.

Anontools
Header
Logout
Contact-Form

Cypress Test-

Header-Testing if the logo-img is visible
Footer-Checks if the id-#footer itself is visible
Contact-Form-Clicking on the Contact link in the footer and typing in the parameters
Logout- Log in ,then logging out the application ,then log in again .

netlify · 2023-06-04T03:17:51Z

✅ Deploy Preview for volto canceled.

Name	Link
🔨 Latest commit	`0a12754`
🔍 Latest deploy log	https://app.netlify.com/sites/volto/deploys/647d8109897cec00070e0f96

sneridagh · 2023-06-05T21:06:45Z

@Tishasoumya-02

Thanks for the PR!

I realised that you didn't have permissions on the Plone repo, so I added you in the "Contributors" Team. (/cc @fredvd)

This will allow you to work with branches in the Volto repo itself from now on. This is how we normally work, this way it's more handy and agile.

I'd like that for every refactored component you open a PR for it. So one PR, one refactored component. This will help us also to keep track of things.

I've completed this PR (I've mentioned you there too): #4767 I'd like you to proceed like this one for all the project's PRs.

Remove all cruft (useless comments, etc)
Remove React import (the build does not need that anymore)
Use the façade pattern (no Redux hooks in the components), we will create a façade hook trying to maintain the reducers folder naming, and creating a hook for every "useSelect" (data retrieval from the Redux store) using common sense in there, as the usage asks for. (Let's comment this out in our upcoming meeting)
We should maintain the resultant HTML. In the case that this is not true, we should have a good reason for it, and issue a breaking change with it.
Test the component in the most effective way if some components is enough using unit tests, lets do that. It's important the Cypress test in components that we are refactoring the lifecycle, to make sure we maintain the same behavior. But for components that does not have lifecycle, it's ok to have only unit tests (like the Header one).
StoryBook, of all the components refactored, showing the instantiation, and add docs for the props if makes sense and it's useful for other developers.

I'll review now the PR, so you can open a PR for every component in the Volto repo branch with the amendments already.

Thanks a lot!

sneridagh

@Tishasoumya-02 I quickly reviewed the PR, please proceed as the previous comment, and then let's discuss further things when we meet.

It might seem a lot of amendments, but we are in the good path, good work so far!

sneridagh · 2023-06-05T21:08:02Z

.github/ISSUE_TEMPLATE/PLIP.md

@@ -4,7 +4,6 @@ about: Plone Improvement Proposal
 title: ''
 labels: ''
 assignees: ''
-


As rule of thumb, do not touch (an edit is present) a file that has nothing to do with the PR. The changes should contain only changes relevant to the current matter.

sneridagh · 2023-06-05T21:08:31Z

.github/workflows/docs.yml

@@ -46,4 +46,3 @@ jobs:
          # Required, set by GitHub actions automatically:
          # https://docs.github.com/en/actions/security-guides/automatic-token-authentication#about-the-github_token-secret
          GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}
-


Same as above, probably you have a formatter that you don't need.

sneridagh · 2023-06-05T21:08:47Z

.github/workflows/unit.yml

@@ -16,7 +16,7 @@ jobs:
      - name: Use Node.js ${{ matrix.node-version }}
        uses: actions/setup-node@v3
        with:
-        # remove workaround for 18.x once https://github.com/nodejs/node/issues/47563 is fixed
+          # remove workaround for 18.x once https://github.com/nodejs/node/issues/47563 is fixed


Same as above, probably you have a formatter that you don't need that is formatting .yml files.

sneridagh · 2023-06-05T21:09:18Z

.storybook/main.js

@@ -48,80 +48,105 @@ const defaultRazzleOptions = {
  staticCssInDev: false,
  emitOnErrors: false,
  disableWebpackbar: false,
-  browserslist: ['>1%', 'last 4 versions', 'Firefox ESR', 'not ie 11', 'not dead']
+  browserslist: [


Although the change seems legit, it would go to it's own PR. It seems that this file never was formatted properly, and the CI is not catching it.

sneridagh · 2023-06-05T21:11:07Z

cypress.config.js

@@ -1,4 +1,4 @@
-const { defineConfig } = require('cypress');
+const { defineConfig } = require('cypress')


In this case, take a look at your Prettier extension, it seems something is off there. This one shouldn't be modified if the extension is reading the Prettier project config.

sneridagh · 2023-06-05T21:16:29Z

src/components/theme/Anontools/Anontools.jsx

-      '@id': null,
-    },
-  };
+    shallowEqual,


Is shallowEqual really necessary here? Since the value is not an object.

sneridagh · 2023-06-05T21:18:37Z

src/components/theme/ContactForm/ContactForm.jsx

-  };
+  const pathname = useMemo(() => {
+    location.pathname;
+  }, [location]);


ESlint is complaining. You are returning nothing.

sneridagh · 2023-06-05T21:18:50Z

src/components/theme/ContactForm/ContactForm.jsx

-  componentDidMount() {
-    this.setState({ isClient: true });
-  }
+  }, [loaded, loading]);



sneridagh · 2023-06-05T21:19:06Z

src/components/theme/Header/Header.jsx

@@ -3,10 +3,10 @@
 * @module components/theme/Header/Header


We will merge the PR from #4767

sneridagh · 2023-06-05T21:21:03Z

src/components/theme/Anontools/Anontools.jsx

-    content: PropTypes.shape({
-      '@id': PropTypes.string,
+const Anontools = () => {
+  const { token, content } = useSelector(


Let's talk about how to make this using the façade, but ideally, we would use one hook for the token, other for the content.

Also, let's discuss if we always return a value or an object. Just to make sure that we are consistent.

Ohkay ,understood

Tishasoumya-02 · 2023-06-06T05:24:50Z

Sure,I will look into these points and make the changes, closing this PR.

Tishasoumya-02 added 3 commits June 4, 2023 08:33

Refactored Anontools,Header,ContactForm,Logout

fd26f37

Refactored Anontools,Header,ContactForm,Logout

65ac06a

Added Cypress test

ea9886a

Tishasoumya-02 added 5 commits June 4, 2023 11:30

Prettier styled

1ebe6ee

Added file to /news folder

4de32ed

Added useCallback,useMemo hooks to Contact-form

2211678

Added useCallback,useMemo hooks to Contact-form

8c34d7e

Prettier

0a12754

sneridagh requested changes Jun 5, 2023

View reviewed changes

Tishasoumya-02 closed this Jun 6, 2023

Tishasoumya-02 deleted the TishaSoumya branch June 6, 2023 05:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored theme components and added cypress test. #4843

Refactored theme components and added cypress test. #4843

Tishasoumya-02 commented Jun 4, 2023

netlify bot commented Jun 4, 2023 •

edited

Loading

sneridagh commented Jun 5, 2023

sneridagh left a comment

sneridagh Jun 5, 2023

sneridagh Jun 5, 2023

sneridagh Jun 5, 2023

sneridagh Jun 5, 2023

sneridagh Jun 5, 2023

sneridagh Jun 5, 2023

sneridagh Jun 5, 2023

sneridagh Jun 5, 2023

sneridagh Jun 5, 2023

sneridagh Jun 5, 2023

Tishasoumya-02 Jun 6, 2023

Tishasoumya-02 commented Jun 6, 2023 •

edited

Loading

		@@ -1,4 +1,4 @@
		const { defineConfig } = require('cypress');
		const { defineConfig } = require('cypress')

Refactored theme components and added cypress test. #4843

Refactored theme components and added cypress test. #4843

Conversation

Tishasoumya-02 commented Jun 4, 2023

Refactored Components

Cypress Test-

netlify bot commented Jun 4, 2023 • edited Loading

✅ Deploy Preview for volto canceled.

sneridagh commented Jun 5, 2023

sneridagh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tishasoumya-02 commented Jun 6, 2023 • edited Loading

netlify bot commented Jun 4, 2023 •

edited

Loading

Tishasoumya-02 commented Jun 6, 2023 •

edited

Loading