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 6 #5230

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

Html specs corrections 6 #5230

wants to merge 8 commits into from

Conversation

TeyaVes
Copy link
Contributor

@TeyaVes TeyaVes commented Nov 13, 2024

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

This PR targets the following items from the related issue:

Splitter

  • some suites (Angular, React) are having k-touch-action-none/ touch-action: none; class/style on the splitbar, do we want to add that class in the reference rendering, or add a style in the themes? -
    the touch-action: none style is added to the themes and the utility class is no longer needed. It needs to be removed from the styles.

Gantt

  • simplify templates, remove tasks
  • check if k-selectable can be removed
  • TreeList does not have k-grid-aria-root

PivotGrid/Checkbox

  • missing k-sr-only on the empty-cell
  • the class on the chip list should be k-column-fields, instead of k-filter-fields
  • k-pivotgrid-content cell value wrapper, do we need this, check if it can be removed.
    I think that we should leave this wrapper with the class- in this way we can target the text inside the cell, without affecting the toggle icon which might be present. Similarly, we have a wrapper for the text inside pivotGrid headers- .k-pivotgrid-header-title

Inputs

  • should opened inputs be k-focused?
    The purpose of the templates are to give guidelines for the static reference rendering of the componets. Adding states to inputs under certain conditions should not be in the scope of the specs. Thus, the inputs when a dropdown is opened will currently remain unchanged.

AI Prompt

  • some products use k-white-space-pre-line utility in the card body. Is this okay, should we consider adding those util styles in the component. -  
    I think that we should not add such a class as a part of the rendering or a style in the component, as it imposes certain styles to the card body content and we do not want that- the client should be responsible for that
  • The toolbar buttons does not have k-toolbar-button classes. Should they be added for consistency with other components?

@TeyaVes TeyaVes self-assigned this Nov 13, 2024
@TeyaVes TeyaVes force-pushed the html-specs-corrections-6 branch 3 times, most recently from 82d18d9 to d6b54ce Compare November 14, 2024 12:09
@TeyaVes TeyaVes added the Test issues related to our visual / ci tests label Nov 14, 2024
@TeyaVes TeyaVes marked this pull request as ready for review November 14, 2024 12:30
@TeyaVes TeyaVes requested a review from a team as a code owner November 14, 2024 12:30
@TeyaVes TeyaVes requested review from kendo-bot and a team and removed request for a team and kendo-bot November 14, 2024 12:30
@TeyaVes TeyaVes added this to the 2024 Q4 (Nov) milestone Nov 15, 2024
@inikolova inikolova self-requested a review November 18, 2024 14:13
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.

LGTM

@inikolova
Copy link
Contributor

Looks OK :) Just one suggestion from me - simplify Gantt templates - should be chore commit;

@TeyaVes
Copy link
Contributor Author

TeyaVes commented Nov 19, 2024

Looks OK :) Just one suggestion from me - simplify Gantt templates - should be chore commit;

I've updated the commit message to chore() instead of fix()

@@ -71,7 +71,7 @@ export const PivotGridWithConfiguratorOpened = ({ formOrientation = "vertical",
<div className="k-form-field">
<label className="k-label">Values</label>
</div>
<ChipList className="k-filter-fields">
<ChipList className="k-column-fields">
Copy link
Contributor

Choose a reason for hiding this comment

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

To summarize, currently we have three fields: rows, columns, and filters.
image
and we provide 3 different classes for each field:

.k-row-fields,
.k-column-fields,
.k-filter-fields {
margin-top: $kendo-pivotgrid-configurator-fields-margin-y;
flex-wrap: wrap;
}

I am uncertain if the k-filter-fields class on the "Values" chip list should be replaced with k-column-fields.
If this is the case k-filder-fields becomes redundant and should be removed from styles.

I also question whether three different classes are necessary for each of the fields.

All these classes share the same styles, so from a styling perspective, it doesn't matter which class is used.
If there isn't a specific need for a different class for each field, we can simplify the approach by using one generic class to handle those field styles. This would reduce confusion and ensure consistency across all fields.

Copy link
Contributor Author

@TeyaVes TeyaVes Nov 21, 2024

Choose a reason for hiding this comment

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

You are absolutely correct, in the general case the correct classes are- fields, rows and columns. However, in the jQuery old PivotGrid implementation (PivotGrid v.1) the styles differ a bit from the PivotGrid v.2 used in the rest of the suites. Updating the classes would impact the styles of the PivotGrid configurator by removing the grey border around the chiplist. Thus, let's leave all classes k-column-fields in the jQuery configurator.

The point you are raising whether we need these different classes is valid since they all share the same styles. I have logged the issue under the Discussion section in the Templates&Spec fixes issue just to make sure that we are not missing something - I am not certain why the fields were named with 3 different class names in the first place.

@TeyaVes TeyaVes force-pushed the html-specs-corrections-6 branch 2 times, most recently from 9b245bd to f721dc5 Compare November 20, 2024 15:20
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