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

[ADD][16.0] developer_menu module #878

Merged
merged 1 commit into from May 14, 2024
Merged

Conversation

bealdav
Copy link
Member

@bealdav bealdav commented May 8, 2024

image

@bealdav
Copy link
Member Author

bealdav commented May 8, 2024

Copy link

@Kev-Roche Kev-Roche left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

I think it's better to move menu entries into the new développer menu, instead of duplicate.

Otherwise, all things will be duplicated. (In the search feature of web_responsive module )

<menuitem
id="conf_tech"
parent="base.menu_administration"
name="🧰"
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no images in menu item in odoo and OCA modules. Introducing it breaks that implicit rule.

Also an image is less accessible and doens't brings any advantages.

Copy link
Member Author

@bealdav bealdav May 9, 2024

Choose a reason for hiding this comment

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

It's not an image, it's a unicode char like emoji. Odoo use emojis in some context.
We are no more in ascii or latin world ;-)

The purpose of OCA is not to always follow the Odoo way in all contexts : there are many examples. That's the key for innovation

The advantage is not to be more accessible, it's to be more compact (in settings apps which is not practical), it's intentional.

This module is not supposed to be installed on production instance as its name tells then no confusion for users.

I think it's better to move menu entries into the new développer menu, instead of duplicate.

I don't think on my side, it's a matter of taste but if someone miss this shortcut , it expects to find orginal menus.

Copy link
Contributor

Choose a reason for hiding this comment

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

The advantage is not to be more accessible

Well, i'm not blind and i'm not very aware of such topic. But IFAIK, set an emoji as the name of an item is not accessible for blind people. Also it can not be searched...
Généraly, using image introduces a lot of matter of taste. For exemple, the current wallet image doesn't mean "configuration" for me, but for you it does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, what do you suggest instead of this wallet aka tool box ?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Développer"

Copy link
Member Author

@bealdav bealdav May 10, 2024

Choose a reason for hiding this comment

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

This i not a wallet but https://emojipedia.org/fr/bo%C3%AEte-%C3%A0-outils

image

Are you agree with 🧰 Dev ? Easy to find because of color and meaningful and short then happy with responsive.

@legalsylvain
Copy link
Contributor

Well. I don't understand this module, and I find the design not in the spirit of odoo. (Duplicating entries, use icon in menu, ...)
However, i'll not block. I let maintainers merge this PR if they found it interesting.

Regards

@bealdav
Copy link
Member Author

bealdav commented May 10, 2024

Thanks for your final words. However it seems your requested blocked merge with change requested. Is it possible to remove request and consider it as comment ?

Thanks a lot

Copy link
Contributor

@mourad-ehm mourad-ehm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@PaulGoubert PaulGoubert left a comment

Choose a reason for hiding this comment

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

Working well and providing direct access to the main technical menus required for functional usage. BTW, the use of icons becomes quite intuitive fairly quickly.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@bguillot
Copy link

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-878-by-bguillot-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 40cf5eb into OCA:16.0 May 14, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 389fd38. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants