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

[IMP] web, board, spreadsheet: add app icon & normalize the UI app icons implementation #163328

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Brieuc-brd
Copy link
Contributor

@Brieuc-brd Brieuc-brd commented Apr 25, 2024

spreadsheet: Add app icon

This PR introduces an app icon for spreadsheet.

Normalize the UI app icons implementation

Prior to this PR, the style related to the UI app icons was defined in each module, which creates duplicated code.
To avoid that, this commit normalizes the implementation of these icons.
Only one class need to be defined: o_ui_app_icon should be defined on the svg.

task-3884720

Requires:


Capture d’écran 2024-04-26 à 08 50 20

I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Apr 25, 2024

@C3POdoo C3POdoo added the RD research & development, internal work label Apr 25, 2024
@Brieuc-brd
Copy link
Contributor Author

Hello @stefanorigano 👋
These PRs (COM & ENT) are ready for review, could you have a look at them ?

@Brieuc-brd Brieuc-brd force-pushed the master-spreadsheet-add-app-icon-brd branch 2 times, most recently from 0ff6574 to a3cb878 Compare April 29, 2024 07:30
@pedrobaeza
Copy link
Collaborator

Taking the occasion, but going a bit off-topic (sorry for that), do you have any general guidelines about the creation of the new milk app icons (color scheme, main layout, etc)?

@Brieuc-brd
Copy link
Contributor Author

Hello @pedrobaeza,
Indeed, we have guidelines for milk icons created for our designers, but these are internal guidelines.
If you need a specific icon, you can check out our brand assets page :)

@pedrobaeza
Copy link
Collaborator

Thanks for answering, Brieuc.

I'm talking on behalf of OCA, as we need to design totally new icons for new apps. Would you ask if that guides can be released for the general public? That way, we would keep consistency between them, as they are all shown in enterprise app chooser and module list. Example:

imagen

@Brieuc-brd
Copy link
Contributor Author

For now I don't know if there are any plans to publish a general guideline related to the new app icons.
But I understand your need and please be assured that we will take your feedback into account 🙂.

cc. @stefanorigano

@Brieuc-brd Brieuc-brd force-pushed the master-spreadsheet-add-app-icon-brd branch from a3cb878 to fd5d9aa Compare May 15, 2024 13:02
Copy link
Contributor

@stefanorigano stefanorigano left a comment

Choose a reason for hiding this comment

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

Hei @Brieuc-brd , thanks so much for your work (and patience) 🙂
Here some remarks for you, thanks again!

addons/web/static/src/scss/ui.scss Outdated Show resolved Hide resolved
addons/web/static/src/scss/ui.scss Outdated Show resolved Hide resolved
@Brieuc-brd Brieuc-brd force-pushed the master-spreadsheet-add-app-icon-brd branch 2 times, most recently from cc87f7b to b7e8435 Compare May 29, 2024 12:36
Copy link
Contributor

@stefanorigano stefanorigano left a comment

Choose a reason for hiding this comment

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

Hello @Brieuc-brd , thanks!
Like the enterprise PR, let's update the pr description and then we're ready to go 👍

Also, nitpicking but we could use the actual module name (board), rather than having it replaced by * in [IMP] web, *: [...] ... it's just a few characters difference at the end.

Thanks!

@Brieuc-brd Brieuc-brd force-pushed the master-spreadsheet-add-app-icon-brd branch from b7e8435 to 6f6082e Compare May 30, 2024 09:38
@Brieuc-brd Brieuc-brd changed the title [IMP] spreadsheet: add app icon [IMP] web, board, spreadsheet: add app icon & normalize the UI app icons implementation May 30, 2024
@Brieuc-brd
Copy link
Contributor Author

Thanks @stefanorigano 🙏
I updated both branches & PR titles & descriptions, could you have a look ?
Thanks again !

Copy link
Contributor

@stefanorigano stefanorigano left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Brieuc-brd 👍

@Brieuc-brd Brieuc-brd marked this pull request as ready for review May 30, 2024 11:37
@Brieuc-brd Brieuc-brd requested a review from a team May 30, 2024 11:38
@Brieuc-brd
Copy link
Contributor Author

Hello @odoo/rd-spreadsheet 👋
Both PR (COM & ENT) are ready for tech. review, could you take a look at them ? :)
Thanks in advance !

@C3POdoo C3POdoo requested review from a team, kebeclibre and Iucapad and removed request for a team May 30, 2024 11:39
@C3POdoo C3POdoo requested a review from a team May 30, 2024 11:39
Prior to this commit, the style related to the UI app icons was defined
in each module, which creates duplicated code.
To avoid that, this commit normalizes the implementation of these icons.

Only one class need to be defined:
- `o_ui_app_icon` should be defined on the svg.

task-3884720
This commit introduces an app icon for `spreadsheet`.

task-3884720
@Brieuc-brd Brieuc-brd force-pushed the master-spreadsheet-add-app-icon-brd branch from 6f6082e to f44e93b Compare June 6, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants