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.

[petschki]

May I ask about the progress here? I'd really like to have this in Plone 6.2 ...

[pgrunewald]

Lots of personal topics came up in my life. I will resume working on this soonish. I have nearly all fixes sitting there to be pushed.

[petschki]

a note to the commitlint error. since @patternslib/dev==3.7.0 theres amake upgrade which removes husky commit hook ... maybe that helps ...

[pgrunewald]

Just to give an update: I will work on this before and maybe while the upcoming Beethoven Sprint.

[pgrunewald]

I think the PR is ready to be reviewed again. There's also a PR for plone.app.locales collective/plone.app.locales#516 that introduces the new msgids and the German translation.

[petschki]

This is great! Tested it on folders with 5k+ items and works like a charm.

Though we need some more reviews on this ...

I'd suggest to release this as new feature version mockup==5.4.0 and in plone.staticresources==2.4.0 and include it in buildout.coredev#6.2 constraints/versions (with a mention in README, that its safely backwards compatible to Plone 6.1 too)

thoughts @plone/classicui-team

[1letter]

i have tested it locally. LGTM. Only my checkout of plone.app.locales don't work with the translations, but this can might be my fault on my pip based mxdev install path

[petschki]

Only my checkout of plone.app.locales don't work with the translations, but this can might be my fault on my pip based mxdev install path

That's likely to your browsers localstorage, where the mockup translations are stored. try removing all the widgets- prefixed localstorage entries and reload the page ... that helped for me ...

[pgrunewald]

There are already two approvals for this PR and the other PR for plone.app.locales is merged as well. Is this PR ready to be merged, too? Would be great to have this in master.

[petschki]

As I said, I would make a new feature release for mockup (5.4.0) ... I only wanted to hear some feedback if this can go into staticresources=2.2.x (Plone 6.1) or we need a feature release there too, which further leads to maintenance branches here. Some overhead, but we could handle this ...

/cc @MrTango @thet @yurj @mauritsvanrees

[mauritsvanrees]

I tried it in 6.1, and it works nicely. Thanks!

I am inclined to say it is fine to include in a Plone 6.1 bug fix release. I suppose there is some chance of breakage, at least breakage in expectation if someone was relying on the old behavior, but I would say that for most people this simply solves a bug.

[petschki]

Thanks @mauritsvanrees ... so to be safe, we could bump staticresources to 2.3.0 with this feature and include it in Plone 6.1 ... if someone is really relying on the old behavior, then staticresources 2.2.x is still there...

[mauritsvanrees]

@petschki Yes, that sounds good.

[petschki]

@pgrunewald I rebased your branch and now the tests are breaking. Have you fixed the structure tests? Did I remove something? Sorry for this!

[pgrunewald]

I'll take a look it and repair it right away if I can.

[pgrunewald]

@petschki I rebased onto master from my local version. Tests run through again. :)

[petschki]

@petschki I rebased onto master from my local version. Tests run through again. :)

Thanks!

[petschki]

I'll prep a PR in staticresources ... there might break some robottests which needed to be fixed before releasing it.

[1letter]

I'll prep a PR in staticresources ... there might break some robottests which needed to be fixed before releasing it.

If you need help with the robottest, i have free timeslots tomorrow.

[petschki]

@1letter great. See plone/plone.staticresources#384

[1letter]

@petschki

Done, see: plone/plone.staticresources#384 and plone/Products.CMFPlone#4193