Skip to content

Components API Review #3107

Closed
Closed

Description

@ilhan007

The goal is to go trough all the components API and check:

  • if the naming could be better
  • if the property/event/slot make sense at all
  • if we should go with a slot, instead of property or the opposite
  • if the event parameters are fine
  • other

Activity

  1. pinned this issue on Apr 7, 2021
  2. fifoosid commented on Apr 8, 2021

    @fifoosid
    Contributor

    Global

    1.1

    • Discussion: Decide on header/subHeader vs heading/subHeading vs title/subtitle vs text/description vs default slot/description vs heading/subtitle vs primaryTitle/SecondaryTitle.
    • Solution:
    • All: use titleText and subtitleText in all components where the text has meaning of title and subtitle
    • All: use headerText and footerText in all components where header and footer slots are present.
    • Panel: use headerText and headerLevel (FIX: refactor(ui5-panel): use heading and headingLevel #3271)
    • ProductSwitchItem: use titleText and subtitleText (FIX: refactor(ui5-product-switch-item): rename heading/subtitle to titleText/subtitleText #3270)
    • TimeLineItem: use titleText and subtitleText
    • ShellBar: keep primaryTitle and secondaryTitle
    • Reason: The titleText and subtitleText are closest to title and subtitle and we preferred this pair over the heading and subheading, as headings are usually meant for entire page or section. Where header and footer element are present, we would like to to have headerText and footerText. When it comes to the ShellBar's primaryTitle and seconadaryTitle, we think that they have special meaning and both are titles, not title and subtitle and this is the reason to keep them as they are.

    1.2

    • Discussion: Decide on disabled/readonly states for all components. Is readonly needed?
    • Solution: Keep readonly
    • Reason: The readonly state is an important state of the components, both disabled and readonly should remain.

    1.3

    • Discussion: Remove busy property where a user can use the default slot of the ui5-busy-indicator(NotificationListItemBase, List, Table, TableRow)
    • Solution: Keep the current implementation.
    • Reason: The app devs can't use the ui5-busy-indicator, because the List, Table has too complex UX related to the growing feature, that requires special handling of the indicator positioning.

    1.4

    • Discussion: Don't use the component tag name in the JS Doc of properties, events, etc.
    • Solution: Use "component" everywhere in the JSDoc, instead of mentioning the component tag name. (Fix).
    • Reason: The tag names that belong to base classes will appear in derived classes and will confuse the readers.

    Avatar

    1.1

    • Discussion: Replace image property with image slot (currently people can't pass properties to the img tag - that's why we added image-fit-type).
    • Solution: Introduce new img slot (Fix).
    • Reason: Adds more flexibility and option to use different types of images, custom icons, etc. + styling using standard CSS.

    1.2

    • Discussion: Replace icon property with icon slot and initials property with initials slot (if we have slots only as an external API, then we can get rid of the properties background-color, size and shape, which are CSS properties as well).
    • Solution: Keep the current implementation.
    • Reason: We will introduce too much overhead for the developer to display and position an icon or initials.

    1.3

    • Discussion: the backgroundColor property sounds like the CSS style property and might mislead users that all types of colors are accepted, such as: "red", "green", "#d34343", etc.
    • Solution: backgroundColor property -> colorScheme (Fix).
    • Reason: to make it consistent with Badge as it follows the same accent colors schema.

    Avatar Group

    • Discussion: is avatarSize needed? The size can be set on each item passed in the default slot.
    • Solution: remove it (Fix).
    • Reason: In frameworks like Angular, React, a loop of items is rendered based on single item template, that's why avatarSize on AvatarGroup is not so valuable, as the Avatar has already avatarSize property that can be used.

    Button

    • Discussion: Replace iconSize property with a CSS Shadow part (only used internally by MessageStrip).
    • Solution: Remove the property and introduce a CSS Shadow Part (Fix) and later adopt the part in the MessageStrip.
    • Reason: It's an obsolete API, that otherwise has to be supported.

    Carousel

    • Discussion: hideNavigation property -> two separate properties (Carousel: Behavior improvement #3136) - one that shows/hides the dots
      and one that shows/hides the arrows (If arrows are in the navigation, then this property would be needed if only the previous is set to true, otherwise the whole navigation will be hidden, containing the arrows).
    • Solution: rename hideNavigation to hideNavigationArrows (Fix).
    • Reason: more specific name to emphasise that the arrows visibility is determined by this property, while the "dots" visibility is controlled by another property.

    CheckBox

    • Discussion: Renametext property to default slot.
    • Solution: Keep the current implementation.

    ColorPalette

    ComboBox: @MapTo0 assigned

    • Discussion: Rename filterValue property to liveValue.
    • Solution: Both properties will be removed

    DatePicker/DateRangePicker/TimePicker:

    • Discussion: The oDate parameter in the formatValue method -> date parameter (No Hungarian Notation).
    • Solution: Use date Fix.

    Duration Picker @vladitasev assigned [refactor Duration Picker API]

    • Discussion: Must not have formatPattern property (inherited from TimePickerBase).
    • Discussion: Must not have formatValue method (also inherited).

    Icon

    Input

    1.1

    • Discussion: Rename maxlength property to maxLength.
    • Solution: Keep the current implementation.
    • Reason: The maxlength is the term used for native HTML elements.

    1.2

    • Discussion: Rename isOpen method to isValueStateOpened.
    • Solution: Hide and rename Fix
    • Reason: The method checks if the value state popover is open.

    SuggestionItem

    • Discussion: Remove group property and introduce a separate component, which has default slot for SuggestionItem (As NotificationListGroupItem & NotificationListItem).
    • Solution: Introduce separate component SuggestionGroupItem (FIX feat(ui5-input): introduce new SuggestionGroupItem #3248).
    • Reason: SuggestionGroupItem is far more simple than the SuggestionItem and inherits too much properties that do not make sense.

    Link

    1.1

    1.2

    • Discussion: Is design property needed? It is basically changing 2 CSS properties.
    • Solution: Keep current implementation
    • Reason: Not so easy to implement the states with CSS as it looks and we would like to keep the property.

    List

    • Discussion: Rename inset property to indent.
    • Solution: rename to indent (Fix)
    • Reason: The word inset means when something is part of, inside something bigger, indent is more accurate.

    ListItem (and derived components)

    • Discussion: Rename info to additionalText.
    • Solution: (Fix)
    • Reason: The term info is too generic as everything is info in some form and additionalText is used in openUI5 as text that is complimentary to the main text.

    MultiComboBoxItem

    • Discussion: Rename additionalText property to info
    • Solution: keep theadditionalText property
    • Reason: The term info is too generic as everything is info in some form and additionalText is used in openUI5 as text that is complimentary to the main text.

    NotificationListItems

    • Discussion: footnotes slot -> footer
    • Solution: footnotes is ok for a name. If for some reason the UX/VD can't be full-filled because of the slot, we can always introduce 2 properties in addition.

    ResponsivePopover

    • Discussion: Should not have openBy method in the docs.
    • Solution: Remove open and use openBy by overriding it to handle the mobile use-case. Fix
    • Reason: To be more consistent with the Popover's openBy, it's better to have openBy.

    ShellBar

    • Discussion: Rename notificationCount property to notificationsCount property.
    • Solution: Rename notificationsCount (Fix).
    • Reason: To have consistency with showNotifications property.

    Switch

    • Discussion: Replace graphical property (boolean) with enum property.
    • Solution: Change to design="Textual, Graphical (Fix).
    • Reason: More future proof - other options can be easily added in future.

    TabContainer

    • Discussion: The tabLayout property is NOT about the layout of the tab. It is about the layout of the header of the tab. Think of more accurate name.
    • Solution: Keep the current implementation
    • Reason: In fact, the tabLayout targets the layout of each tab - the position of the text, icon and additional-text.

    Table

    1.1

    • Discussion: Replace moreText property with a slot (we already had this use case, it works fine, until developers need to change a couple of attribute of the button)
    • Solution: Rename to growingButtonText (Fix refactor(ui5-table): more(sub)Text replaced by growingButton(sub)Text #3273)
    • Reason: Keep the property approach, instead of slot, because it would require significant styling as the growing button has a special design.

    1.2

    1.3

    • Discussion: Rename showNoData property to hideNoData property (this way, by default, the no data text will be shown)
    • Solution: Rename to ```hideNoData`` (Fix)
    • Reason: The default value should be consistent with the UX specs.

    TextArea

    • Discussion: Rename maxlength property to maxLength.
    • Solution: Keep the current implementation.
    • Reason: The maxlength is the term used for native HTML elements.

    TimeLineItem:

    • titleName, titleText, itemNameClickable properties & item-name-click event -> single slot
    • Solution: Rename itemName to name, itemNameClickable to nameClickable
    • Reason: Preferred the props to keep the enterprise flavour of this component as it is very specific to Fiori 3
  3. added this to the 1.0.0-rc.15 milestone on Apr 9, 2021
  4. ilhan007 commented on Apr 13, 2021

    @ilhan007
    ContributorAuthor

    All Components

    1.1

    • Discussion: The slot names are camelCase, shouldn't they be kebab-case as the events and attributes?
    • Solution: Keep the current implementation.
    • Reason: The slot names are the values of the slot attribute so they do not have to be kebab-case, but more important the internal usage would become a bit uglier - Instead of this.suggestionItems, developers will have to write this["suggestion-items"].

    1.2

    • Discussion: Some CSS Shadow Part names are camelCase, we have to use kebab-case.
    • Solution: Rename CSS Shadow Part names to kebab-case.
    • Reason: In the Web, all CSS Shadow Parts are kebab-case.

    1.3

    • Discussion: Some of the components have subheading, some have "subtitle-text", others subtitle. Should be unified (keep in mind that title is a reserved word).
    • Solution:
    • All: use titleText and subtitleText in all components where the text has meaning of title and subtitle
    • All: use headerText and footerText in all components where header and footer slots are present.
    • Panel: use headerText and headerLevel (FIX: refactor(ui5-panel): use heading and headingLevel #3271)
    • ProductSwitchItem: use titleText and subtitleText (FIX: refactor(ui5-product-switch-item): rename heading/subtitle to titleText/subtitleText #3270)
    • TimeLineItem: use titleText and subtitleText
    • ShellBar: keep primaryTitle and secondaryTitle
    • Reason: The titleText and subtitleText are closest to title and subtitle and we preferred this pair over the heading and subheading, as headings are usually meant for entire page or section. Where header and footer element are present, we would like to to have headerText and footerText. When it comes to the ShellBar's primaryTitle and secondaryTitle, we think that they have special meaning and both are titles, not title and subtitle and this is the reason to keep them as they are.

    1.4

    • Discussion: The properties type and design sound generic, we need to be consistent in using the one or the other. The Bar, Page, Button and Link use design, while the AvatarGroup, Input, ListItem, TableRow, MessageStrip use type.
    • Solution: Use design for visual appearance, while type for functional modes. From the components above, only the MessageStrip's type has to be renamed to design (Fix)

    1.5

    Button

    • Discussion: The iconSize property seems to be added for the purpose of the MessageStrip's close button,
      it is currently public, perhaps we should make it protected/private. Or, if really needed perhaps we need to introduce a slot / CSS Shadow Part
    • Solution: Remove the property and introduce a CSS Shadow Part (Fix) and later adopt the part in the MessageStrip.
    • Reason: It's an obsolete API, that otherwise has to be supported.

    ComboBox

    1.1

    • Discussion: loading property should be renamed to busy.
    • Solution: Keep the current implementation.
    • Reason: The loading serves different purpose, it shows a loading indicator over the items in the popover, the user can keep typing. While busy is showing over the component and prevents typing.

    1.2

    ColorPalette

    Carousel [discuss with team Rodopi]

    • Discussion: Carousel hide-navigation property is confusing.
    • On desktop the navigation & arrows are hidden and here is no way for users to scroll.
    • On mobile: The arrows are hidden by default on mobile(no way to show them), but the items are scrollable left & right (the default mobile behaviour) sap.m.Carousel not always shows the arrow buttons for navigation (mobile) openui5#2826.
    • Solution: The component is implemented by spec, just the docs should be more clear. Add more description about the UX of the component when it comes to navigation on desktop and mobile:
      On desktop - the user can navigation with LEFT/RIGHT Arrows or via the navigation arrows
      On touch devices, the user can use the swipe gesture to navigate through the items. By spec, the navigation arrows are not displayed on touch devices. Fix docs(Switch, Carousel): improve docs #3395

    Card [discuss with team Rodopi]

    • Discussion: Discuss the slot "action", could be be removed in favour of slot "header".
    • Solution: Introduce a new slot "header" for the Card and new component CardHeader to serve as a header (with all the props/slot related to the header part) refactor(ui5-card): header slot is added #3490

    ProductSwitchItem

    • Discussion: The component have "heading" property, and at the same time "subtitle".
    • Solution: Use titleText and subtitleText

    RangeSlider

    • Discussion: We have "starValue" and "endValue", but at the same time "min" (the min value) and "max" (the max value). Isn't more consistent to have either "start, end, min, max", or "startValue, endValue, minValue and maxValue"
    • Solution: Use startValue, endValue, min and max Fix
    • Native Input (type="number") has "min" and "max".

    RatingIndicator, Slider and StepInput

    • Discussion: RatingIndicator, Slider and StepInput components all have "value" property.
      But, the RatingIndicator has "maxValue" in addition, while the Slider and StepInput has "max" property in addition.
      We have to unify those by naming them both "max" or "maxValue" (the Slider/StepInput have also "min").
    • Solution: RatingIndicator maxValue to become "max" Fix
    • Native Input (type="number") has "min" and "max".

    RadioButton

    1.1

    • Discussion: Rename the "select" event to "change", f.e. the CheckBox (Switch, RatingIndicator, ColorPalette) has "change" event, and in general it's more used name.
    • Solution: Rename the "select" event to "change" Fix

    1.2

    • Discussion: Rename the "selected" property to "checked" in order to be consistent with CheckBox, Switch and the HTML terminology.
    • Solution: Rename the "selected" event to `"checked" Fix

    Select

    SegmentedButton

    • Discussion: Discuss the tag name ui5-segmentedbutton or ui5-segmented-button
    • Solution: Rename to ui5-segmented-button Fix

    Tab

    ToggleButton

    • Discussion: Discuss the tag name ui5-togglebutton or ui5-toggle-button
    • Solution: Rename to ui5-toggle-button Fix

    Popover/ResponsivePopover

    • Discussion: Popover has openBy(opener), ResponsivePopover has open(opener) - should be unified
    • Solution: Use openBy for both components Fix

    Toast

    • Discussion: The public method "show()" can be renamed to "open()" to be more consistent to Popover's openBy, ResponsivePopover
    • Solution: As we are going to rename the Dialog's open to show, this should remain.

    TimelineItem

    • Discussion: Suggestions for renaming the props "itemName" to "name" and "itemNameClickable" to "nameClickable"
    • Solution: Rename "itemName" to "name" and "itemNameClickable" to "nameClickable" Fix refactor(ui5-timeline-item): rename API #3387
    • Reason: The word item is obsolete as the component has item in its name.
  5. nnaydenow commented on Apr 21, 2021

    @nnaydenow
    Contributor

    AvatarGroup

    1

    • Discussion: Think about replacing ui5-avatar with ui5-avatar-group-item (or something else). To many properties were introduced into the API of ui5-avatar so it can have correct interaction and accessibility behavior so it can fit as item in ui5-avatar-group. Previously it was display only control.

    • Exmples
      The Interactive and focus properties. They were added because ui5-avatar should be accessed via keyboard in ui5-avatar-group context and it should have click event

      Duplication of size and backgroundColor property. They gives the possibility to managed ui5-avatar internally in ui5-avatar-group

    • Solution: Keep the current implementation.

    2

    • Discussion:
    • Solution: Remove avatar-size, make use of Avatar size and define the size of the overflow btn according to the first slotted item.
  6. 91 remaining items

  7. ilhan007 commented on Jul 21, 2021

    @ilhan007
    ContributorAuthor

    Component API Review has been completed.

    Moreover, most of the required changes (~60+) are already merged (2 in review at the of writing) and 1 still not started: #3519

    You can track all the breaking changes, that are result from the major API Review here:
    https://github.com/SAP/ui5-webcomponents/pulls?q=is%3Apr+is%3Aopen+label%3ABREAKING_CHANGE

  8. unpinned this issue on Jul 21, 2021
  9. moved this to 1.0.0-rc.15 (released Aug) in UI5 Web Components - Roadmapon Jun 20, 2024
  10. moved this to 1.0.0-rc.15 (released Aug) in UI5 Web Components - Roadmapon Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Type

No type

Projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions