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] theme_orchid: revamp theme #948

Open
wants to merge 1 commit into
base: 18.0
Choose a base branch
from

Conversation

stefanorigano
Copy link
Contributor

@stefanorigano stefanorigano commented Sep 26, 2024

task-4178087
part of task-4177975

17.4 this PR
image image

@robodoo
Copy link
Collaborator

robodoo commented Sep 26, 2024

Pull request status dashboard

@stefanorigano stefanorigano changed the base branch from 18.0 to master September 26, 2024 13:46
@stefanorigano stefanorigano marked this pull request as ready for review September 26, 2024 13:46
@stefanorigano stefanorigano marked this pull request as draft September 26, 2024 13:46
@stefanorigano stefanorigano changed the base branch from master to 18.0 September 26, 2024 13:47
@stefanorigano stefanorigano force-pushed the master-theme_orchid-revamp-sri branch 2 times, most recently from 4b68250 to 5e71bba Compare September 26, 2024 16:19
@stefanorigano stefanorigano marked this pull request as ready for review September 26, 2024 16:44
Copy link

@agau-odoo agau-odoo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! :)

Here are some comments, including a bunch of nitpicking:

image

-> this transition doesn't match the jpg preview, + it seems odd to me. It's even more apparent on bigger screens (i.e. 1440p)
The shape is cut + the white color is slightly different
The different color is also visible in the jpg preview

theme_orchid/static/description/theme_orchid.svg Outdated Show resolved Hide resolved
theme_orchid/static/src/scss/bootstrap_overridden.scss Outdated Show resolved Hide resolved
theme_orchid/static/src/scss/primary_variables.scss Outdated Show resolved Hide resolved
theme_orchid/views/snippets/s_freegrid.xml Outdated Show resolved Hide resolved
theme_orchid/views/snippets/s_key_images.xml Outdated Show resolved Hide resolved
theme_orchid/views/snippets/s_key_images.xml Outdated Show resolved Hide resolved
theme_orchid/views/snippets/s_kickoff.xml Show resolved Hide resolved
theme_orchid/views/snippets/s_media_list.xml Outdated Show resolved Hide resolved
theme_orchid/views/snippets/s_process_steps.xml Outdated Show resolved Hide resolved
theme_orchid/views/snippets/s_references.xml Outdated Show resolved Hide resolved
@stefanorigano stefanorigano force-pushed the master-theme_orchid-revamp-sri branch 2 times, most recently from ec45dec to 9b58b1c Compare September 27, 2024 12:58
@stefanorigano
Copy link
Contributor Author

Hello @agau-odoo thanks so much for your review 👍
Changes applied and green in runbot (the very last push it's just about the SVG).

Thanks!

Copy link

@agau-odoo agau-odoo left a comment

Choose a reason for hiding this comment

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

Here are some more comments, mostly nitpicking again, but still a few important issues imo. We're almost there!

Choose a reason for hiding this comment

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

Okay but now we get this on chromium:
image

And this on firefox:
image

I think we should prefer targeting chromium users first (biggest userbase), so if we don't have an easy fix now, let's revert to the previous version and fix this later.

The issue with the different color at the bottom part of this shape is still there
image, but again I don't know if this is important

Choose a reason for hiding this comment

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

There's also a weird line here (firefox):
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now I see. The issue it's that the font it's not loaded correctly on your side.
It should be "Elsie", see @import rule in the svg file.
Could you tell tell me if you get error in your console? Somehow the font it's not used and this leads to discrepancies... in my side it's good on Firefox, Chrome and Safari (OSX)

image

Choose a reason for hiding this comment

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

image

The color is fixed, but the shape still clips on a 1080p screen (tested in chromium)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but the shape still clips on a 1080p screen (tested in chromium)

Can't be solved until we introduce responsive margin; the solution would have been to add pt256, but this value it's totally unsuitable for mobile.

I'll remove the shape.. I don't mind it, it's clean:

image

theme_orchid/views/snippets/s_key_images.xml Outdated Show resolved Hide resolved
theme_orchid/views/snippets/s_kickoff.xml Outdated Show resolved Hide resolved
theme_orchid/views/snippets/s_kickoff.xml Outdated Show resolved Hide resolved
@stefanorigano
Copy link
Contributor Author

@agau-odoo , shape removed and screenshoot/svg updated.
Nitpicking remarks addressed as well. Let me know for the font, thanks.

task-4178087
part of task-4177975
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants