telerik / kendo-themes

Monorepo for SASS-based Kendo UI themes
149 stars 71 forks source link

Tabstrip design revision #5158

Closed silviyaboteva closed 2 months ago

silviyaboteva commented 2 months ago

Closes: https://github.com/telerik/kendo-themes-private/issues/267

inikolova commented 2 months ago

@silviyaboteva great work :) Please find my suggestions below:

inikolova commented 2 months ago

One more thing, Angular TabStrip has a tabAlignment option for tabs alignment already implemented - in my opinion it is a good practice to use the same property names (where possible) across all suites. Hence my suggestion is to rename the alignment prop to tabAlignment (or tabsAlignment).

silviyaboteva commented 2 months ago

One more thing, Angular TabStrip has a tabAlignment option for tabs alignment already implemented - in my opinion it is a good practice to use the same property names (where possible) across all suites. Hence my suggestion is to rename the alignment prop to tabAlignment (or tabsAlignment).

Done! Thanks! <3

inikolova commented 2 months ago

One final thing which I noticed and believe we should consider. The tabbed column menu in Grid uses Tabstrip with icons only (without text). The rendering there suggest that the icon is inside the ".k-link-text" wrapper: https://github.com/telerik/kendo-themes/blob/tabstrip-revision/tests/grid/grid-column-menu-tabbed.html#L38 The current rendering of the Tabstrip component suggests that the icon and the "k-link-text" elements are siblings https://github.com/telerik/kendo-themes/blob/tabstrip-revision/tests/tabstrip/tabstrip-icons.html#L29 In my opinion a change in the rendering is needed and both components should have the same HTML

What do you think about this suggestion?

silviyaboteva commented 2 months ago

@inikolova Thank you for this catch!

✅ As we now have the option of passing text only or icon only, or both, I'll go ahead and update the Grid Column menu test to use the new icon prop. It will result in correct rendering as the icon element should not be rendered as a child of k-link-text but as a sibling prior or after the text (if iconPosition is passed). This is synced with @zhpenkov and @Juveniel

✅ Also, from the sync with @Juveniel, I have added actions prop which allows tabstrip item to have more than one action (for example in Dock Manager where there are pin and more actions). The actions will be rendered inside k-item-actions element including the remove button.

@zhpenkov when you have the chance, please check it :)