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

Html specs corrections 5 #5227

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Html specs corrections 5 #5227

wants to merge 10 commits into from

Conversation

TeyaVes
Copy link
Contributor

@TeyaVes TeyaVes commented Nov 11, 2024

targets: https://github.com/telerik/kendo-themes-private/issues/246

This PR targets the following items from the related issue:

Toolbar

  • see if Angular-specific toolbar specs/templates can be removed.

Map

  • k-button-square is missing on navigator buttons
    It seems that the class is obsolete - can we remove it?

Expansion panel / Expander

  • should use k-hidden instead of k-d-none when not expanded

Switch

  • add offLabel and onLabel in the switch templates

TabStrip

  • check if we can remove k-header class
    Removed from the spec as the class seems to be obsolete
  • we should use k-tabstrip-item instead of k-item
    Both classes would be present for now as the styles in the themes are attached to k-item class and some in some suites only the k-item class is present, thus replacing it would impose a breaking change
  • check if we can remove k-tab-on-top class as mentioned here -  Unify rendering for ColumnMenu
    Removed from the spec as the class seems to be obsolete

CheckBox

  • should checked CheckBoxes have k-checked class
    both the class k-checked and the state :checked hold the styles. However, we should keep the class for consistency - we are adding state classes for buttons, inputs, etc- k-hover, k-focus, etc. So following the same logic, we should add k-check to the checked checkox as well .
  • add a template with checked CheckBoxes

ImageEditor

  • The form label "Lock aspect ratio:" should become a label of the checkbox, following the design guidelines.

DateTimePicker, others

  • add missing k-time-accept, k-time-cancel to adaptive buttons
  • check if we can replace k-calendar-container and k-list-container with k-datetime-container - ref

@TeyaVes TeyaVes self-assigned this Nov 11, 2024
@TeyaVes TeyaVes added the Test issues related to our visual / ci tests label Nov 11, 2024
@TeyaVes TeyaVes added this to the 2024 Q4 (Nov) milestone Nov 11, 2024
@TeyaVes TeyaVes force-pushed the html-specs-corrections-5 branch 4 times, most recently from d0d1438 to 504f046 Compare November 12, 2024 15:16
@TeyaVes TeyaVes marked this pull request as ready for review November 12, 2024 16:12
@TeyaVes TeyaVes requested a review from a team as a code owner November 12, 2024 16:12
@Juveniel Juveniel modified the milestone: 2024 Q4 (Nov) Nov 13, 2024
@inikolova inikolova self-requested a review November 18, 2024 13:40
@inikolova
Copy link
Contributor

Most of the things look OK :) A small adjustment is needed in the ImageEditor - there is width 241px set to .k-imageeditor-action-pane - this width should be increased, as in the Material and Fluent themes there is a horizontal scrollbar.

Copy link
Contributor

@epetrow epetrow left a comment

Choose a reason for hiding this comment

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

Are the changes in the ImageEditor expected as it does not look good in some of the themes?

@zhpenkov zhpenkov self-requested a review November 19, 2024 08:26
@TeyaVes TeyaVes force-pushed the html-specs-corrections-5 branch 2 times, most recently from dac6f0f to 619177d Compare November 19, 2024 16:11
@Juveniel Juveniel modified the milestones: 2024 Q4 (Nov), 2025 Q1 (Feb) Nov 20, 2024
@TeyaVes
Copy link
Contributor Author

TeyaVes commented Nov 20, 2024

As discussed at the FE & Design sync, I'll fix the width of the ImageEditor Pane for now and apply any further changes once the design has been revised. However, I'll do so in a separate PR for a better clarity: #5238

Copy link
Contributor

@epetrow epetrow left a comment

Choose a reason for hiding this comment

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

Looks okay overall, I am just wondering about two things:

  1. If we are removing the k-tab-on-top class, shouldn't we remove it from spreadsheet-jquery.tsx as well? And if it is an obsolete class should't we delete it?
  2. Are we okay with the difference in the ImageEditor label font weight in Fluent? I know that this is the correct checkbox position, but still.

@TeyaVes
Copy link
Contributor Author

TeyaVes commented Nov 21, 2024

Looks okay overall, I am just wondering about two things:

  1. If we are removing the k-tab-on-top class, shouldn't we remove it from spreadsheet-jquery.tsx as well? And if it is an obsolete class should't we delete it?
  2. Are we okay with the difference in the ImageEditor label font weight in Fluent? I know that this is the correct checkbox position, but still.
  1. I've removed the k-tab-on-top class from the styles and the specs.

  2. I think that we should leave the checkbox with label with the normal font weight in Fluent - following the logic only the formfield labels are bold.

Copy link
Contributor

@zhpenkov zhpenkov left a comment

Choose a reason for hiding this comment

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

Looking Good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Test issues related to our visual / ci tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants