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

Sort control panels alphabetically by title within each group #3737

Merged
merged 17 commits into from
Nov 7, 2023

Conversation

JeffersonBledsoe
Copy link
Member

@JeffersonBledsoe JeffersonBledsoe commented Oct 6, 2022

The control panels within each group are sorted alphabetically

@netlify
Copy link

netlify bot commented Oct 6, 2022

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 988502d
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/654923ac4888620008572c05
😎 Deploy Preview https://deploy-preview-3737--volto.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@cypress
Copy link

cypress bot commented Oct 6, 2022

Passing run #5107 ↗︎

0 493 20 0 Flakiness 0

Details:

Merge branch 'master' into sort-controlpanel-by-title
Project: Volto Commit: 4cc3886c1f
Status: Passed Duration: 14:36 💡
Started: May 11, 2023 3:24 PM Ended: May 11, 2023 3:38 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@stevepiercy
Copy link
Collaborator

Will sorting work for all languages?

Will the sort be within groups of control panels?

@erral
Copy link
Member

erral commented Oct 7, 2022

AFAIK if you are using the title they will be sorted by title, because p.restapi takes care of exposing the translated title in the relevant controlpanel. But I can provide some screenshot in some languages to check it.

@sneridagh
Copy link
Member

We tried this in the past (in classic, I contributed to it), and it does not work, because of languages. People preferred to have the icon in the same place across languages than have them alphabetically ordered.

@erral
Copy link
Member

erral commented Oct 7, 2022

But we already fixed it some time ago in classic to have it sorted by title:

We should have it both in classic and volto in the same way. I don't know which one (I prefer have it sorted) but the same.

@sneridagh
Copy link
Member

I do not know now the state in classic, but I agree that we should follow the same approach.

@JeffersonBledsoe JeffersonBledsoe changed the title Sort control panels alphabetically by title Sort control panels alphabetically by title within each group Oct 7, 2022
@JeffersonBledsoe
Copy link
Member Author

I updated the PR title to reflect that it's sorted within each group.

if you are using the title they will be sorted by title

@erral That was my understanding too, but I haven't verified this yet.

People preferred to have the icon in the same place across languages than have them alphabetically ordered.

@sneridagh My experience with multi-lingual sites is very limited, but I imagine most users would use a single language most of the time? I agree that it's not ideal having them move in cases when they do switch, but I can't think of another way right now for them to be logically sorted the other 80% of the time. I'll finish this PR off, can always revisit it :)

@erral

This comment was marked as outdated.

@davisagli
Copy link
Member

Hmm, why are there different numbers of control panels shown in the different languages?

@erral
Copy link
Member

erral commented Oct 10, 2022

Good question 🤔 I think that's because I didn't change the Volto language when testing this, I only modified Plone language, but I will check it again

@erral
Copy link
Member

erral commented Oct 10, 2022

French

controlpanels-fr-v2

Basque

controlpanels-eu-v2

Spanish

controlpanel-es-v2

Here we have the screenshots with frontend + backend both configured correctly with the same language.

The controlpanel differences in French are because of some controlpanels are directly coded in Volto and are not comming from Plone, so their category indicator must be translated into French in the PO/JSON file and it is not. If you check it carefully we have a category called Général (coming from Plone) and the one below called General which comes from Volto and it is not translated yet.

@erral
Copy link
Member

erral commented Oct 15, 2022

@JeffersonBledsoe can you add a changelog entry so that the build is green?

@JeffersonBledsoe
Copy link
Member Author

@erral I'll sort that shortly, but I've still some tests failing locally

@JeffersonBledsoe
Copy link
Member Author

@tiberiuichim Update the snapshot and did some more testing, all finished :)

@JeffersonBledsoe JeffersonBledsoe requested review from sneridagh and removed request for sneridagh November 17, 2022 12:56
@JeffersonBledsoe
Copy link
Member Author

@sneridagh Thoughts on including this in the Plone6 release?

Copy link
Member

@erral erral left a comment

Choose a reason for hiding this comment

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

👍

JeffersonBledsoe and others added 4 commits November 18, 2022 10:45
* master: (185 commits)
  fix: unresponsive add page (#4507)
  Apply suggestion from browser for password field (#4524)
  (fix):Object.normaliseMail: Cannot read properties of null (#4558)
  Fix link in Volto, remove from linkcheck ignore in Documentation v6.0 (#4742)
  added documentation regarding the static middleware #4518 (#4736)
  Closes issue #4567 (#4570)
  Fix whitespace in locales created by the generator (#4737)
  Tidy up from synch with 16.x.x (#4728)
  Release notes from 16.20.2, 16.20.3, 16.20.4 (#4729)
  Use new URL 6.docs.plone.org (#4726)
  Security upgrade for momentjs (#4715)
  Don't decode querystring while adding apiExpanders (#4719)
  (Synchronize redundant block id in listing block on pasting) Fix duplicating listing block (#4239)
  Translate add-on control panel (cleaned up PR) (#4620)
  Fix Move to top of folder ordering in folder content view by searchin… (#4709)
  Fix robot.txt - the sitemap link should respect x-forwarded headers (#4704)
  Refactor faulty reorder elements in ObjectBrowserList widget (#4703)
  Release changelog notes for 16.20.1
  Release 17.0.0-alpha.5
  Generate a split sitemap (also fix robots.txt) (#4639)
  ...
@sneridagh
Copy link
Member

@JeffersonBledsoe upss I merged your other PR related to the SSR control panel and now there is a conflict that must be resolved.

# Conflicts:
#	src/components/manage/Controlpanels/Controlpanels.jsx
@JeffersonBledsoe
Copy link
Member Author

JeffersonBledsoe commented May 11, 2023

@sneridagh Thanks for merging that other PR! I've fixed the merge conflict here, just waiting for tests to run. Are you fussed about the import sorting that VS Code seems to have done, or want me to undo it for a cleaner PR?

@tkimnguyen
Copy link
Member

I have reached my (very low) limit of comprehension about why Controlpanels.test.jsx is problematic :)

@sneridagh
Copy link
Member

Doh, clear example of buried PR... should we resurect it?
BTW, I took a quick look, so we are sorting by (English - not translated GS property) title? Also in classic?

@davisagli
Copy link
Member

@sneridagh I think we should do it. It sorts on the translated title (it's translated in the @controlpanels API). It doesn't make sense to sort in English if we're displaying a different language.

@sneridagh
Copy link
Member

Ok I just checked and the endpoint return the translations. @davisagli ok! let's do it.

@sneridagh sneridagh merged commit 3843434 into main Nov 7, 2023
48 checks passed
@sneridagh sneridagh deleted the sort-controlpanel-by-title branch November 7, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants