dbeck121 / CPI-Helper-Chrome-Extension

58 stars 20 forks source link

Design adaptations #175

Closed fippu82 closed 6 months ago

fippu82 commented 6 months ago

@incpi I have changed the position units from px to rem. But I can't figure out how to change the default color and change the black theme color to something else. Can you please help with this or point me to the right direction?

DevGregor commented 6 months ago

@fippu82 We set the background color for the header bar here: image We need to make sure to change the text color of all the child nodes to something bright since SAP has made the header bar colorwise inverted in the new UI. We also don't have to worry about "the old UI" if we change the text color because it is already something bright.

Old UI (still used in CF currently) -> already has bright text color by SAP's default image

New UI (only in Neo atm) -> has dark text color by SAP's default image

Incpi commented 6 months ago

Hi @fippu82 /@DevGregor ,

No. Don't change this at all. Logic inside popup.js need to change most probably.

There is property cpi custom color in css just change default blue to #fff.

for user selected color you might need to tweak logic for auto color adjusters to restrict by dark colors. Right now it is restricted by bright colors. Conditions reverse.

Might need to remove black option at all.

fippu82 commented 6 months ago

Thanks for your inputs. Yes, I tried to change the logic in popup.js (where the color selector is) but it somehow had no impact. Will look into it again later.

DevGregor commented 6 months ago

Thanks for your inputs. Yes, I tried to change the logic in popup.js (where the color selector is) but it somehow had no impact. Will look into it again later.

@fippu82 / @incpi But take a look at line 174-175. This sets that property "--cpi-custom-color" so I think this overwrites it: image

The color is defined in the variable hostData: image

Incpi commented 6 months ago

Hi,

You are right, It does overwrite...

But no need to change it, just replace the 2nd snapshot value with #ffffff

It's not easy to just flip the value but it needs a bit more attention. It has a lot of impact here and there

DevGregor commented 6 months ago

Hi,

You are right, It does overwrite...

But no need to change it, just replace the 2nd snapshot value with #ffffff

It's not easy to just flip the value but it needs a bit more attention. It has a lot of impact here and there

I don't think this will work for 2 reasons:

Functions are executed in this order: image getHostData function checks the storage and overwrites hostData if there is something stored already: image Notice that within getHostData we also call setData and if you go down the line it will eventually set the header bar color with the function setHeaderColor (first screenshot of my first comment)

Incpi commented 6 months ago

Hi, No color goes with black & white at a same time.Preset can go but look we can extend dynamiclly as per below request. Need to change lot of stuff, we are extending for new + old UI at the same time? No right...getHostData leave it as it is.. No need to change that. @fippu82 please look this Pull request

fippu82 commented 6 months ago

Hi guys, thank you @incpi for the changes you already made in my fork. However, I'm not sure what we are actually trying to achieve. In my opinion, we have only these two rather small problems:

  1. If somebody has the dark blue or black color activated today -> when their layout changes to the new SAP UI, they can hardly read the icons (and the title "Cloud Integration") on the header.

The easy solution for the user would be to change the theme to another color. From our side: we have removed black from the color selector.

  1. The "Reset Color" button defaults to dark blue (old SAP UI theme) -> here we just set it to something that works on both SAP UI themes for now, e.g. (light) blue. Later, when all tenants are moved to the new UI theme, we can set it to white which is the default header color of the new UI.

That's a pragmatic and simple approach.

Let me know if I'm missing something.

Incpi commented 6 months ago

Hi,

My goal, by the time we complete the changes, SAP will likely release it in the CF horizon theme.

Let's maintain backward compatibility. Instead of white, let's use light blue.

Additionally,

In my opinion, we should create a CSS property that changes dynamically, rather than making extensive changes to the HTML, JS, etc. That's why I suggested the function to do it.

Is anything still unclear? Or anything Let me know.

DevGregor commented 6 months ago

New UI seems to have been rolled out in CF too now. @fippu82 / @incpi

fippu82 commented 6 months ago

Yes, in my opinion, we should merge this PR and release it soon (@dbeck121). It fixes the location of the sidebar and improves the colors (thanks to @incpi).
We can then still further improve what's necessary.

dbeck121 commented 6 months ago

Perfect! I will care for next release

@fippu82 could you add changes to changelog?