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

refactor(ui5-input): style icon slot content by default #8995

Closed
wants to merge 9 commits into from

Conversation

nnaydenow
Copy link
Contributor

@nnaydenow nnaydenow commented May 17, 2024

With this change, all icons slotted inside icon slot of ui5-input are visually enhanced for hover/pressed state by default.

Related to: #8461

@github-actions github-actions bot added the Stale label Jun 8, 2024
@nnaydenow nnaydenow removed the Stale label Jun 11, 2024
@nnaydenow nnaydenow requested a review from MapTo0 June 11, 2024 07:59
@nnaydenow nnaydenow changed the title WIP: ui5-input icon styles refactor(ui5-input): style icon slot content by default Jun 20, 2024
@MapTo0
Copy link
Member

MapTo0 commented Jun 20, 2024

looks good to me

@MapTo0 MapTo0 requested a review from elenastoyanovaa June 20, 2024 13:12
@nnaydenow nnaydenow requested a review from ilhan007 July 1, 2024 06:37
@@ -524,6 +524,8 @@ class Input extends UI5Element implements SuggestionComponent, IFormInputElement

/**
* Defines the icon to be displayed in the component.
*
* **Note:** It is recommended to hide the icon when the component is in readonly state
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO there will also be cases for users to add decorative icons intentionally (to serve as a visual label) and this statement is not okay in the case. Better to try to fix the issues for the readonly case than restrict the user.

packages/main/src/Input.ts Show resolved Hide resolved
packages/main/src/MultiComboBox.hbs Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
@import "./FormComponents.css";
@import "./InvisibleTextStyles.css";
@import "./InputSharedStyles.css";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a big change and my opinion on that is that it should not be done here as part of the icon styling as it does not have have a direct relationship. No need to complicate the change and have the risk of potentially introducing issues in all inputs for the release as it would be impossible to test all input components thoroughly in a short matter.

It is the correct way to go, but not as a 2.0 change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though it would be good since we are doing "css refactoring" somehow by moving all icon related styles to separate file. Basically the file contain only 1:1 css code that is duplicated in both files (Input.css and InputSharedStyles.css). If you do global search in your IDE for the file content of InputSharedStyles it will match the content in Input.css.

}

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

packages/main/src/themes/InputIcon.css Outdated Show resolved Hide resolved
--_ui5_input_error_warning_custom_icon_padding: .1875rem .5rem;
--_ui5_input_error_warning_custom_focused_icon_padding: .1875rem .5rem;
--_ui5_input_information_error_warning_custom_icon_padding: .1875rem .5rem;
--_ui5_input_information_error_warning_custom_focused_icon_padding: .1875rem .5rem;
--_ui5_input_information_custom_icon_padding: .1875rem .5rem;
Copy link
Contributor

@ndeshev ndeshev Jul 1, 2024

Choose a reason for hiding this comment

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

_ui5_input_information_custom_icon_padding has different values from the error and warning ones in the high-contrast Fiori 3 themes.

Also as it is, both --_ui5_input_information_custom_icon_padding and --_ui5_input_information_custom_focused_icon_padding params are not used anywhere

nnaydenow added a commit that referenced this pull request Jul 2, 2024
Icon styles related to the `ui5-input` component will now be implemented by enhancing slotted elements (see: #8995). This change eliminates the need for CSS modules setup, as there are no remaining candidates requiring this approach.

Additionally, the configuration to use CSS modules in somed technologies is excluded by default, making the setup process more complex (e.g., requiring specific configurations for webpack or other bundlers). In most cases, such styles are intended to be utilized via CDN if available, further reducing the necessity for CSS modules.
@nnaydenow nnaydenow closed this Jul 17, 2024
@nnaydenow nnaydenow deleted the icons-input-1 branch August 2, 2024 06:53
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