-
Notifications
You must be signed in to change notification settings - Fork 71
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
base: develop
Are you sure you want to change the base?
Html specs corrections 6 #5230
Conversation
82d18d9
to
d6b54ce
Compare
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.
LGTM
Looks OK :) Just one suggestion from me - simplify Gantt templates - should be chore commit; |
300196f
to
39b6368
Compare
I've updated the commit message to chore() instead of fix() |
30134c1
to
ce1df77
Compare
@@ -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"> |
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.
To summarize, currently we have three fields: rows, columns, and filters.
and we provide 3 different classes for each field:
kendo-themes/packages/default/scss/pivotgrid/_layout.scss
Lines 245 to 250 in 7e81c25
.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.
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.
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.
9b245bd
to
f721dc5
Compare
fbf29db
to
83c9c21
Compare
The touch-action none is now enforced through the themes rather than with a utility class
83c9c21
to
7ffdead
Compare
targets: https://github.com/telerik/kendo-themes-private/issues/246
This PR targets the following items from the related issue:
Splitter
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
PivotGrid/Checkbox
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
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
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