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

fix(ui5-progress-indicator): hidden value design #8545

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

PetyaMarkovaBogdanova
Copy link
Contributor

Component padding reduced in case with hidden value.

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

For the other themes, this parameter --_ui5_progress_indicator_padding_novalue won't exist and basically it will be padding-top: undefined.

Even if this works and does not cause issue in the other themes, if we follow this pattern to only define variables for the themes they were initially required, soon it will get difficult to maintain the css params, because some of them will be defined only in these themes, some of them only in these themes.

That's why so far we used to define the variables always by adding them to the base params with some default value that is fine for most of the themes, and then overwrite them where needed (in this case sap_horizon, would not be also needed in sap_horizon_dark - they are usually pretty close in padding/margins). This way when you see a parameter, it's guaranteed to exist in all themes and you won't need to track it down and see where it comes from.

@PetyaMarkovaBogdanova
Copy link
Contributor Author

For the other themes, this parameter --_ui5_progress_indicator_padding_novalue won't exist and basically it will be padding-top: undefined.

Even if this works and does not cause issue in the other themes, if we follow this pattern to only define variables for the themes they were initially required, soon it will get difficult to maintain the css params, because some of them will be defined only in these themes, some of them only in these themes.

That's why so far we used to define the variables always by adding them to the base params with some default value that is fine for most of the themes, and then overwrite them where needed (in this case sap_horizon, would not be also needed in sap_horizon_dark - they are usually pretty close in padding/margins). This way when you see a parameter, it's guaranteed to exist in all themes and you won't need to track it down and see where it comes from.

Thanks, Ilhan, for the informative remark.

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

last check: should not we also add the --_ui5_progress_indicator_padding_novalue: 0.3125rem; int sap_horizon_dark?

@PetyaMarkovaBogdanova
Copy link
Contributor Author

last check: should not we also add the --_ui5_progress_indicator_padding_novalue: 0.3125rem; int sap_horizon_dark?

Horizon_dark theme seems to inherit parameters for ProgressIndicator from the main horizon theme (which imports also layout parameters), so for now I do not see the need to add it there also.

@ilhan007
Copy link
Member

ilhan007 commented Apr 17, 2024

Checked the case - it works in sap_horizon_dark without any additional changes, because in the sap_horizon_dark/parameters-bundle.css you are basically importing the sap_horizon params:

// sap_horizon_dark/parameters-bundle.css
@import "../sap_horizon/ProgressIndicator-parameters.css";

Although this is the only case done like this (only the ProgressIndicator) - it's fine to remain as it is. So no action items in that regards.


One unrelated type that I found accidentally why looking into the change (could be fixed in a separate change I don't mind),

I noticed this variable usage in the ProgressIndicator.css (line 3) while reviewing this change:
min-height: var(_ui5_progress_indicator_host_min_height); without the -- in front,

it should be:
min-height: var(--_ui5_progress_indicator_host_min_height);

@PetyaMarkovaBogdanova PetyaMarkovaBogdanova merged commit bf094d3 into main Apr 18, 2024
9 checks passed
@PetyaMarkovaBogdanova PetyaMarkovaBogdanova deleted the pi-responsiveness branch April 18, 2024 07:33
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.

None yet

3 participants