Clear selection when changing path in structure pattern

[pgrunewald]

This fixes plone/Products.CMFPlone#3933

I think persistent selections creates more problems that it solves. With this PR it resets the selection whenever the path is changed.

Let me know, if something more is needed.

As a side note, I had problems with the commit-linter

I had to by-pass the commit-linter via git commit --no-verify -m "...".

This was the error:

❯ npx commitlint --config node_modules/@patternslib/dev/commitlint.config.js --edit
/home/paul/dev/buildout.coredev/src/mockup/node_modules/cosmiconfig-typescript-loader/node_modules/jiti/dist/babel.cjs:244
      `},wrapReference(ref,payload){if("lazy/function"===payload)return lib.types.callExpression(ref,[])}}))(lazy))},visitor:{["CallExpression"+(api.types.importExpression?"|ImportExpression":"")](path){if(path.isCallExpression()&&!lib.types.isImport(path.node.callee))return;let{scope}=path;do{scope.rename("require")}while(scope=scope.parent);transformDynamicImport(path,noInterop,this.file)},Program:{exit(path,state){if(!(0,helper_module_imports_lib.isModule)(path))return;path.scope.rename("exports"),path.scope.rename("module"),path.scope.rename("require"),path.scope.rename("__filename"),path.scope.rename("__dirname"),allowCommonJSExports||(process.env.BABEL_8_BREAKING?(0,helper_simple_access_lib.A)(path,new Set(["module","exports"])):(0,helper_simple_access_lib.A)(path,new Set(["module","exports"]),!1),path.traverse(moduleExportsVisitor,{scope:path.scope}));let moduleName=(0,helper_module_transforms_lib.getModuleName)(this.file.opts,options);moduleName&&(moduleName=lib.types.stringLiteral(moduleName));const hooks=function(file){const hooks=file.get(commonJSHooksKey);return{getWrapperPayload:(...args)=>findMap(hooks,(hook=>hook.getWrapperPayload?.(...args))),wrapReference:(...args)=>findMap(hooks,(hook=>hook.wrapReference?.(...args))),buildRequireWrapper:(...args)=>findMap(hooks,(hook=>hook.buildRequireWrapper?.(...args)))}}(this.file),{meta,headers}=(0,helper_module_transforms_lib.rewriteModuleStatementsAndPrepareHeader)(path,{exportName:"exports",constantReexports,enumerableModuleMeta,strict,strictMode,allowTopLevelThis,noInterop,importInterop,wrapReference:hooks.wrapReference,getWrapperPayload:hooks.getWrapperPayload,esNamespaceOnly:"string"==typeof state.filename&&/\.mjs$/.test(state.filename)?mjsStrictNamespace:strictNamespace,noIncompleteNsImportDetection,filename:this.file.opts.filename});for(const[source,metadata]of meta.source){const loadExpr=async?lib.types.awaitExpression(lib.types.callExpression(lib.types.identifier("jitiImport"),[lib.types.stringLiteral(source)])):lib.types.callExpression(lib.types.identifier("require"),[lib.types.stringLiteral(source)]);let header;if((0,helper_module_transforms_lib.isSideEffectImport)(metadata)){if(lazy&&"function"===metadata.wrap)throw new Error("Assertion failure");header=lib.types.expressionStatement(loadExpr)}else{const init=(0,helper_module_transforms_lib.wrapInterop)(path,loadExpr,metadata.interop)||loadExpr;if(metadata.wrap){const res=hooks.buildRequireWrapper(metadata.name,init,metadata.wrap,metadata.referenced);if(!1===res)continue;header=res}header??=lib.template.statement.ast`
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            

SyntaxError: Unexpected token '??='
    at wrapSafe (internal/modules/cjs/loader.js:1029:16)
    at Module._compile (internal/modules/cjs/loader.js:1078:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1143:10)
    at Module.load (internal/modules/cjs/loader.js:979:32)
    at Function.Module._load (internal/modules/cjs/loader.js:819:12)
    at ModuleWrap.<anonymous> (internal/modules/esm/translators.js:203:29)
    at ModuleJob.run (internal/modules/esm/module_job.js:183:25)
    at async Loader.import (internal/modules/esm/loader.js:178:24)
    at async Object.loadESM (internal/process/esm_loader.js:68:5)
    at async handleMainPromise (internal/modules/run_main.js:59:12)

[petschki]

I'm d'accord with your statement, that the persistent selection creates more problems, than it solves. But it was designed for this purpose and if you clear the selection when changeing path, it doesn't make sense to me at all to show this dropdown anymore.

Nevertheless, this is a breaking change and has to be discussed. @plone/classicui-team

[pgrunewald]

The dropdown still might be useful, when selecting across page 1, page 2, ... within the same folder_contents view. While that being said, we at TUD have customized/simplified the dropdown in the moment, when we introduced this change. We introduced three options:

• Select all items in the folder
• Select all items on this page
• Cancel selection

See the screenshots as better illustration:

English:

German:

I did not want to pitch too much in this PR, but if more improvements are needed (like shown in the screenshots), I'm willing to add this to my PR as well (with the original styles of course).

[petschki]

ah, that would be a very nice improvement. Happy to review the PR then.

Note: since last week we got rid of husky as a commit hook and use native git commit-msg hook to run commitlint on the repo. This needs a make install in you local checkout though. Then the postinstall script removes husky and adds .git/hooks/commit-msg to your repo.

[petschki]

BTW. the commitlint uses conventionalcommit style for generating the CHANGES.md ... so if you would reformat your commit message to

fix(pat structure): Clear selection when changing path in structure pattern

it would appear correctly in the changelog.

[pgrunewald]

I have added the changes. Some notes:

• i opted for the bootstrap icons, but I can use more fitting icons as seen in the initial screenshots. Is this done by putting them in plone.staticresources?
• I added new msgids strings. I'm unsure, when they get into plone.app.locales and how to provide translations right now.
• tests are not updated, since I wanted to wait for some feedback before finishing everything up

I created a small screencast too:

selection-button-demo.mp4

Let me know, what you think.

[yurj]

• I added new msgids strings. I'm unsure, when they get into plone.app.locales and how to provide translations right now.

mockup/README.md

Line 230 in a265e4d

2. Copy the widgets.pot file to plone.app.locales

you've to add them to plone.app.locales, translation can be done there or, after, using weblate: https://hosted.weblate.org/browse/plone/widgets/en/?q=

[1letter]

If i remember me correctly, the work on robottest for the selection button was very tricky. We run in some flaky test results.
For me is the change a improvement.

[petschki]

Huge +1 from me for this improvement 🎉

I've some minor issues here:

• first time I go to folder_contents the button says "0 of 0 selected" ... when I select something, then the of x fits the amount of items in the folder.

• I'm perfectly fine with the bootstrap icons. Introducing new icons would need an upgrade step in staticresources to register them in the registry ... but that's too much noise I'd say.
• tests needs to be adapter accordingly.

[petschki]

If i remember me correctly, the work on robottest for the selection button was very tricky. We run in some flaky test results. For me is the change a improvement.

yes we must also check for robottests in CMFPlone if we need to change something. @pgrunewald are you able to run the playwright browsers and test the dependent robottests ?

[pgrunewald]

Playright tests run generally. Just those tests breaks, where I have changed something. I will work on the remaining tasks (bugs, tests, i18n, docs) starting by tomorrow.