-
Notifications
You must be signed in to change notification settings - Fork 16
Add demo on styling grid with HasTheme
#255
Conversation
src/test/java/com/vaadin/flow/component/grid/demo/GridView.java, line 1095 at r1 (raw file):
I propose to do this with theme variants. In order to do this, you need to:
|
src/test/java/com/vaadin/flow/component/grid/demo/GridView.java, line 1095 at r1 (raw file):
Would it look better if you add some sort of theme toggling? |
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.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @vaadin-bot and @ZheSun88)
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @vaadin-bot and @ZheSun88)
src/test/java/com/vaadin/flow/component/grid/demo/GridView.java, line 1095 at r1 (raw file):
Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
I propose to do this with theme variants.
In order to do this, you need to:
- Add the variants generated: https://github.com/vaadin/flow/blob/master/flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/grid/GridVariant.java
- Add two methods for manilulating the variants: https://github.com/vaadin/flow/blob/master/flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/grid/GeneratedVaadinGrid.java#L308
- Use them here
actually, this PR is wrong.. because the purpose of the demo should be theme the component inside grid..
the necessary part in the PR is only adding the hasTheme
interface. I think i need to close this PR.. ...
src/test/java/com/vaadin/flow/component/grid/demo/GridView.java, line 1095 at r1 (raw file): Previously, ZheSun88 (Sun Zhe) wrote…
Nope, as discussed, you carry on with this one :) |
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.
Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ZheSun88)
SonarQube analysis reported 21 issues Top 10 extra issuesNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
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.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ZheSun88)
a discussion (no related file):
Should we close it or update the same way as in vaadin/vaadin-button-flow#77?
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ZheSun88)
a discussion (no related file):
Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
Should we close it or update the same way as in vaadin/vaadin-button-flow#77?
yes
Part of the ticket vaadin/flow#4318
This change is