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

JSONForms VanillaLayouter does not expose enough CSS classes, which results in heavy code duplication on framework user code #1677

Closed
hoelzl-florian opened this issue Jan 18, 2021 · 4 comments · Fixed by #1679

Comments

@hoelzl-florian
Copy link

Files:
firefox_inspector_result.html.txt - relevant HTML source as shown in Firefox Inspector mode
named-commented-eobject.css.txt - CSS that achieved the result (BUT with heavy code duplication)
model-properties-view-content-widget.react.txt - JSONForms framework user code

Expected behavior
Every DIV section generated by JSONForms should have a proper CSS class that can be overridden by user code.

Screenshots
PropertySection.png - Screenshot of the Solution
PropertySection

Browser:

  • firefox
  • 82.0.2

Used Setup (please complete the following information):

  • Framework: React
  • RendererSet: Vanilla
  • JSONForms: 2.5.0-alpha.1

Based on this information I submit 3 bugs in replies to this initial report.

@hoelzl-florian
Copy link
Author

hoelzl-florian commented Jan 18, 2021

BUG 1:
Refers to:
firefox_inspector_result.html.txt : line 6 (also line 13)
model-properties-view-content-widget.react.txt : 116

Observation: It is possible to inject a custom CSS class "ctk4-model-properties-group-layout-item".
Problem: It's properties are overriden by the more generic CSS class "group-layout-item".
Solution: Swap the order of the CSS classes.

@hoelzl-florian
Copy link
Author

hoelzl-florian commented Jan 18, 2021

BUG 2:
Refers to:
firefox_inspector_result.html.txt : line 10 (also line 17)

Observation: There is no custom CSS class in this element. If there were a CSS class, how would it
distinguish between an imperative message ("Please enter name.") and an error message ("Error: this
is not a name!").
Problem: It is not possible to inject a custom CSS class into the "validation" element.
Solution: Expose this element similar to the input and label element.

@hoelzl-florian
Copy link
Author

hoelzl-florian commented Jan 18, 2021

BUG 3:
Refers to:
firefox_inspector_result.html.txt : line 7 (also line 14)
named-commented-eobject.css.txt : all lines

Observation: CSS classes of this DIV section use names that correspond to the schema / UI schema.
There is no generic class for this section.
Problem: In my case, the schema and UI schema is composed of contributions from different sources,
which might not even know from one another. They all coordinate via the model properties service and
the properties view asks the service what should be displayed. Therefore, every contribution has to
bring its own CSS definition, as can be seen in the CSS file. This file contains heavy CSS code
duplication due to the fact that there is no generic CSS class next to the specific, named-dependent
classes that are already there.
Solution: Also generate a generic class for each item in the JSONForms, e.g., like this
div class="jsonform-property root_properties_comment" id="#/properties/comment"
Then one can save a lot of duplicated CSS code by defining the default behavior / layout with
"jsonform-property" and still override it with "root_properties_comment".

@hoelzl-florian hoelzl-florian changed the title JSONForms VanillaLayouter does not expose enough CSS classes, which results in heavy code duplication JSONForms VanillaLayouter does not expose enough CSS classes, which results in heavy code duplication on framework user code Jan 18, 2021
@sdirix
Copy link
Member

sdirix commented Jan 18, 2021

Thanks for this extensive report!

BUG 1:
Refers to:
firefox_inspector_result.html.txt : line 6 (also line 13)
model-properties-view-content-widget.react.txt : 116

Observation: It is possible to inject a custom CSS class "ctk4-model-properties-group-layout-item".
Problem: It's properties are overriden by the more generic CSS class "group-layout-item".
Solution: Swap the order of the CSS classes.

I agree, we can add the hardcoded group-layout-item at the beginning. For more stability I would recommend to select both classes in your CSS, i.e. group-layout-item.ctk4-model-properties-group-layout-item and therefore have higher prioritization than the standalone default CSS.

BUG 2:
Refers to:
firefox_inspector_result.html.txt : line 10 (also line 17)

Observation: There is no custom CSS class in this element. If there were a CSS class, how would it
distinguish between an imperative message ("Please enter name.") and an error message ("Error: this
is not a name!").
Problem: It is not possible to inject a custom CSS class into the "validation" element.
Solution: Expose this element similar to the input and label element.

Good point, we should definitely do this.

BUG 3:
Refers to:
firefox_inspector_result.html.txt : line 7 (also line 14)
named-commented-eobject.css.txt : all lines

Observation: CSS classes of this DIV section use names that correspond to the schema / UI schema.
There is no generic class for this section.
Problem: In my case, the schema and UI schema is composed of contributions from different sources,
which might not even know from one another. They all coordinate via the model properties service and
the properties view asks the service what should be displayed. Therefore, every contribution has to
bring its own CSS definition, as can be seen in the CSS file. This file contains heavy CSS code
duplication due to the fact that there is no generic CSS class next to the specific, named-dependent
classes that are already there.
Solution: Also generate a generic class for each item in the JSONForms, e.g., like this
div class="jsonform-property root_properties_comment" id="#/properties/comment"
Then one can save a lot of duplicated CSS code by defining the default behavior / layout with
"jsonform-property" and still override it with "root_properties_comment".

Makes sense to me! I would like to suggest the name control as this reflects the actual usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants