-
Notifications
You must be signed in to change notification settings - Fork 281
refactor(ui5-multi-combobox): switch to .tsx #10807
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
Conversation
| onClick={this.togglePopoverByDropdownIcon} | ||
| onMouseDown={this._onIconMousedown} | ||
| onFocusIn={this._forwardFocusToInner} | ||
| // pressed={this.open} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we did this alternative for pressed={this.open} in other components:
"inputIcon--pressed": this.open
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not find any CSS that works with this property. Also, in the original template it was with ? and bound as a property, so there wasn't even an attribute most probably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We switched to a class inputIcon--pressed some time ago, in the InputIcon.css, imported by the Multi here.
.inputIcon.inputIcon--pressed {
background: var(--_ui5_input_icon_pressed_bg);
box-shadow: var(--_ui5_input_icon_box_shadow);
border-inline-start: var(--_ui5_select_hover_icon_left_border);
color: var(--_ui5_input_icon_pressed_color);
}Previously it was like .inputIcon[pressed] (which this pressed={this.open} was for), but was not ts-friendly, and we refactored it when migrating other input-based components like the Combo.
| placeholder={this._getPlaceholder} | ||
| disabled={this.disabled} | ||
| readonly={this.readonly} | ||
| value-state={this.valueState} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value-state attribute on the input element looks like some left over when the Multi used to compose Input web component - @MapTo0 do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking the same, but assumed there was some CSS around it. I'll double check
Edit: I checked but this value-state attribute was not in CSS so it can also be deleted
|
It turned out that there are 4-5 tests that use the |
hristop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 🇮🇹
…ui5-user-menu-tests * 'main' of https://github.com/SAP/ui5-webcomponents: refactor(ui5-multi-combobox): switch to .tsx (#10807) refactor(ListItem): delete unused code (#10796) chore(deps-dev): bump esbuild from 0.19.9 to 0.25.0 (#10816) chore(deps-dev): bump vitest from 3.0.2 to 3.0.5 (#10785) test: add command for single cy test run (#10813) docs: fix TimelineGrowingMode enum docs (#10815) fix(ui5-side-navigation): remove inappropriate exclamation mark usage (#10769)
…ui5-settings * 'main' of https://github.com/SAP/ui5-webcomponents: refactor(ui5-multi-combobox): switch to .tsx (#10807) refactor(ListItem): delete unused code (#10796) chore(deps-dev): bump esbuild from 0.19.9 to 0.25.0 (#10816) chore(deps-dev): bump vitest from 3.0.2 to 3.0.5 (#10785) test: add command for single cy test run (#10813) docs: fix TimelineGrowingMode enum docs (#10815) fix(ui5-side-navigation): remove inappropriate exclamation mark usage (#10769) feat(ui5-link): introduce reactive area size property (#10762) fix(ui5-side-navigation): "Space" key triggers links (#10767) chore(ui5-timeline): importing a timeline group item in the samples (#10793) chore: add ElementInternals shim (#10782) chore: fix MultiInput test (#10791) Translation Delivery (#10772) chore(ui5-select): remove unused css vars (#10789) chore: warn when scoping suffix is set too late (#10781) fix(ui5-button): remove tap highlight on safari (#10786) feat(ui5-table): action header cell is added (#10698) docs: fix List sample chore(release): publish v2.8.0-rc.0 [ci skip]
Findings while doing the transition to
.tsx:not-editableicon was removed for bothComboBoxandMultiComboBox(was not used anywhere). The other 2 icons were just moved to the template.additionalTextfield was added forIMultiComboBoxItemas it was used in the templateMultiComboBoxTemplate.tsxthere was askipRegistryUpdateproperty that does not exist forui5-popover(you can see it commented out)pressedproperty forui5-iconin the same template (also commented out).hbsfile:class="ui5-responsive-popover-header {{classes.popoverValueState}}"which required some minor refactoring in
get classes()