Implement openSidebarSections in redux store #9381

maartenleenders · 2018-03-29T09:35:45Z

Summary

This PR can be summarized in the following changelog entry:

Makes sure you can open multiple keyword sections at the same time.

Relevant technical choices:

Previously activeKeyword kept track of the keyword tab that was opened. We now opted for an array that keeps track of all open sections, which can be multiple.
In comparison to PR 9377, we now leave activeKeywords intact next to openSidebarSections. The activeKeywords fields should be removed when completely moving to the sidebar. See: Remove activeKeywords and related logic when moving to sidebar #9380

Test instructions

This PR can be tested by following these steps:

Checkout branch and run yarn install and yarn test (when the tests have passed CR you can assume they are fine, though do check them).
You could expose the store and action to the window in edit.js, and dispatch (store.dispatch( action() ) and see whether the state was changed correctly.

Quality assurance

I have tested this code to the best of my abilities
I have added unittests to verify the code works as intended

Fixes #9305

abotteram · 2018-03-29T09:50:00Z

js/src/redux/reducers/openSidebarSections.js

+/**
+ * Helper function: returns an array with the requested item removed.
+ *
+ * @param   {Array}  openSections The array from which the item should be removed.


Name doesn't match parameter openSidebarSections

abotteram · 2018-03-29T09:50:07Z

js/src/redux/reducers/openSidebarSections.js

+ * Helper function: returns an array with the requested item removed.
+ *
+ * @param   {Array}  openSections The array from which the item should be removed.
+ * @param   {string} removeItem   The item to be removed.


Name doesn't match parameter removeSection

Also I think the name of the param should just be sectionId. Same goes for sidebarSectionCloser.

Wasn't sure we would use ids, names, indexes, etc.

So I called it item. I'd rather change the action's fields to address your concerns.

abotteram · 2018-03-29T09:50:56Z

js/src/redux/reducers/openSidebarSections.js

+ *
+ * @returns {Array}               The array without the item that should be removed.
+ */
+let sidebarSectionCloser = ( openSidebarSections, removeSection ) => {


Should be const

abotteram · 2018-03-29T09:51:05Z

js/src/redux/reducers/openSidebarSections.js

+	return openSidebarSections.filter( item => item !== removeSection );
+};
+
+let sidebarSectionOpener = ( openSidebarSections, addSection ) => {


Should be const

Doesn't have documentation

abotteram · 2018-03-29T10:14:23Z

js/src/redux/reducers/openSidebarSections.js

@@ -0,0 +1,49 @@
+import { CLOSE_ALL_SIDEBAR_SECTIONS, OPEN_SIDEBAR_SECTION, CLOSE_SIDEBAR_SECTION } from "../actions/openSidebarSections";


If you write functions to handle state change I think it's better to name them after the action instead of their responsibility. So I think sidebarSectionOpener should just be openSidebarSection for example.

I agree, but for readability's sake I used sidebarSectionOpener() because of overlap with the redux action OPEN_SIDEBAR_SECTION and its creator openSidebarSection().

Will consider changing.

abotteram · 2018-03-29T10:16:54Z

js/src/redux/reducers/openSidebarSections.js

+	];
+};
+
+/**


Don't make it more elaborate than it needs to be ;)

/** * A reducer that manages the open state for sections. * * @param {string} state The current state of the object. * @param {Object} action The current action received. * * @returns {string} The state. */

While I agree I went a little overboard here, what I was going for was documenting which part of the state this reducer actually manages. In this file you cannot see this anywhere, you'd have to visit the rootReducer in edit.js. To have everything together, it could be worthwhile to write down the name of the state that is managed per reducer.

abotteram · 2018-03-29T10:30:24Z

Just saying I disagree with the usage of an array in this case.

maartenleenders · 2018-03-29T13:50:55Z

I did too, initially, but @atimmer convinced me. I think for now an array solves the issue adequately.

abotteram · 2018-04-03T07:16:52Z

CR 👍

…ctions

…2-implement-openSections

abotteram · 2018-04-03T08:19:30Z

Acceptance 👍

…openSections" This reverts commit ec9aaf0, reversing changes made to 32913c6. Reverting this redux because we've decided to handle openness of collapsibles in the local state of the collapsibles themselves.

…ns-redux Revert "Merge pull request #9381 from Yoast/stories/9305-2-implement-openSections"

Add openSidebarSections actions, reducers, and tests

271bd23

maartenleenders changed the base branch from trunk to feature/content-analysis-in-sidebar March 29, 2018 09:36

maartenleenders mentioned this pull request Mar 29, 2018

Change activeKeyword-based logic to openSidebarSections-based logic #9377

Closed

2 tasks

abotteram suggested changes Mar 29, 2018

View reviewed changes

maartenleenders added 2 commits March 29, 2018 15:39

Add missing docs and clarify variables to deal with codereview

e3681d6

Remove redundant 'the'

47180ee

abotteram approved these changes Apr 3, 2018

View reviewed changes

Alexander Botteram and others added 2 commits April 3, 2018 10:03

Changed removeSection and addSection to sectionId in sectionSidebar a…

17a51be

…ctions

Merge branch 'feature/content-analysis-in-sidebar' into stories/9305-…

232e690

…2-implement-openSections

abotteram closed this Apr 3, 2018

abotteram reopened this Apr 3, 2018

abotteram merged commit ec9aaf0 into feature/content-analysis-in-sidebar Apr 3, 2018

abotteram deleted the stories/9305-2-implement-openSections branch April 3, 2018 08:19

atimmer added this to the Content Analysis in Sidebar milestone Apr 3, 2018

IreneStr restored the stories/9305-2-implement-openSections branch July 10, 2018 11:19

IreneStr deleted the stories/9305-2-implement-openSections branch July 10, 2018 11:19

IreneStr mentioned this pull request Jul 10, 2018

Revert "Merge pull request #9381 from Yoast/stories/9305-2-implement-openSections" #10309

Merged

1 task

maartenleenders added a commit that referenced this pull request Jul 12, 2018

Merge pull request #10309 from Yoast/stories/revert-openSidebarSectio…

6553517

…ns-redux Revert "Merge pull request #9381 from Yoast/stories/9305-2-implement-openSections"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement openSidebarSections in redux store #9381

Implement openSidebarSections in redux store #9381

maartenleenders commented Mar 29, 2018

abotteram Mar 29, 2018

abotteram Mar 29, 2018

abotteram Mar 29, 2018

maartenleenders Mar 29, 2018 •

edited

Loading

abotteram Mar 29, 2018

abotteram Mar 29, 2018

abotteram Mar 29, 2018

abotteram Mar 29, 2018

maartenleenders Mar 29, 2018

abotteram Mar 29, 2018 •

edited

Loading

maartenleenders Mar 29, 2018

abotteram commented Mar 29, 2018

maartenleenders commented Mar 29, 2018

abotteram commented Apr 3, 2018

abotteram commented Apr 3, 2018

		@@ -0,0 +1,49 @@
		import { CLOSE_ALL_SIDEBAR_SECTIONS, OPEN_SIDEBAR_SECTION, CLOSE_SIDEBAR_SECTION } from "../actions/openSidebarSections";

+              	];
+              };
+              /**

Implement openSidebarSections in redux store #9381

Implement openSidebarSections in redux store #9381

Conversation

maartenleenders commented Mar 29, 2018

Summary

Relevant technical choices:

Test instructions

Quality assurance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maartenleenders Mar 29, 2018 • edited Loading

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

abotteram Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abotteram commented Mar 29, 2018

maartenleenders commented Mar 29, 2018

abotteram commented Apr 3, 2018

abotteram commented Apr 3, 2018

maartenleenders Mar 29, 2018 •

edited

Loading

abotteram Mar 29, 2018 •

edited

Loading