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.

Sign up for GitHub

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

Jump to bottom

Add default loading indicator theme #4174

Merged
merged 5 commits into from
May 29, 2018
Merged

Add default loading indicator theme #4174

merged 5 commits into from
May 29, 2018

Conversation

pleku
Copy link
Contributor

@pleku pleku commented May 25, 2018 •
edited by SomeoneToIgnore

Adds default loading indicator theming, and a way to disable it.
The theming is the same as in Vaadin 8 (Valo).
Makes UI configuration objects available via InitialPageSettings.
Fixes the loading indicator configuration not working from server side (at all), broken since 0.0.1.

Tested on macOS with

  • Chrome, Safari and FF
    Tested on Windows 10 with
  • Chrome, FF, Edge and IE11

Related to #3763

This change is Reviewable

@pleku pleku mentioned this pull request May 25, 2018
Open
vaadin-bot
vaadin-bot reviewed May 25, 2018
View reviewed changes
flow-client/src/main/java/com/vaadin/client/LoadingIndicator.java
@@ -38,6 +37,60 @@

private static final String PRIMARY_STYLE_NAME = "v-loading-indicator";

private static final String DEFAULT_THEMING = "@-webkit-keyframes v-progress-start {" +
"0% {width: 0%;}" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Define a constant instead of duplicating this literal "0% {width: 0%;}" 3 times. rule

vaadin-bot
vaadin-bot reviewed May 25, 2018
View reviewed changes
flow-client/src/main/java/com/vaadin/client/LoadingIndicator.java
"3% {width: 91%;height: 7px;}" +
"100% {width: 96%;height: 7px;}}" +
"@-webkit-keyframes v-progress-wait-pulse {" +
"0% {opacity: 1;}" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Define a constant instead of duplicating this literal "0% {opacity: 1;}" 3 times. rule

vaadin-bot
vaadin-bot reviewed May 25, 2018
View reviewed changes
flow-client/src/main/java/com/vaadin/client/LoadingIndicator.java
@@ -38,6 +37,60 @@

private static final String PRIMARY_STYLE_NAME = "v-loading-indicator";

private static final String DEFAULT_THEMING = "@-webkit-keyframes v-progress-start {" +
"0% {width: 0%;}" +
"100% {width: 50%;}}" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Define a constant instead of duplicating this literal "100% {width: 50%;}}" 3 times. rule

vaadin-bot
vaadin-bot reviewed May 25, 2018
View reviewed changes
flow-client/src/main/java/com/vaadin/client/LoadingIndicator.java
"100% {width: 96%;height: 7px;}}" +
"@-webkit-keyframes v-progress-wait-pulse {" +
"0% {opacity: 1;}" +
"50% {opacity: 0.1;}" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Define a constant instead of duplicating this literal "50% {opacity: 0.1;}" 3 times. rule

vaadin-bot
vaadin-bot reviewed May 25, 2018
View reviewed changes
flow-client/src/main/java/com/vaadin/client/LoadingIndicator.java
"@-webkit-keyframes v-progress-wait-pulse {" +
"0% {opacity: 1;}" +
"50% {opacity: 0.1;}" +
"100% {opacity: 1;}}" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Define a constant instead of duplicating this literal "100% {opacity: 1;}}" 3 times. rule

pleku added a commit to vaadin/flow-and-components-documentation that referenced this pull request May 25, 2018
@pleku
Update loading indicator documentation
0d73479 
Needs vaadin/flow#4174 to be merged before it compiles.
@denis-anisimov
Copy link
Contributor

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.

a discussion (no related file):
It looks like IT test is missing.

flow-client/src/main/java/com/vaadin/client/LoadingIndicator.java, line 291 at r2 (raw file):

defaultThemeApplied

It's better to use getter instead of direct field reference.
Here and below.

flow-client/src/main/java/com/vaadin/client/LoadingIndicator.java, line 306 at r2 (raw file):

setDefaultThemeApplied

defaultThemeApplied is a fact that already happened at some point in the past for me.
So I would say it doesn't express the intention of the flag which is actually "should be applied".
Would be good to find some better name. May be setUseDefaultThemeor shouldUseDefaultTheme (though I don't like should in method names).

flow-server/src/main/java/com/vaadin/flow/component/page/LoadingIndicatorConfiguration.java, line 88 at r2 (raw file):

isDefaultThemeApplied

Not sure whether the meaning of the flag is different here. But most likely it's the same issue with the naming here.

Comments from Reviewable

pleku added 5 commits May 28, 2018 14:20
@pleku
Fix loading indicator configuration
5c0ff22
@pleku
Enable disabling default loading indicator theme
9448d79 
Only adds the server side API for this.
Exposes UI configuration objects in InitialPageSettings for easier discoverability and usage.
@pleku
Add default loading indicator theme
d4c89b2 
Adds default theming for loading indicator, which is the same as in Vaadin 8.
Adds the client parts for disabling the default theme.
@pleku
Add missing javadocs
542fdf7
@pleku
Rename API, add ITs
156520e
@pleku
Copy link
Contributor Author

pleku commented May 28, 2018

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

It looks like IT test is missing.

Done.

flow-client/src/main/java/com/vaadin/client/LoadingIndicator.java, line 291 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…
defaultThemeApplied

It's better to use getter instead of direct field reference.
Here and below.

Done.

flow-client/src/main/java/com/vaadin/client/LoadingIndicator.java, line 306 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…
setDefaultThemeApplied

defaultThemeApplied is a fact that already happened at some point in the past for me.
So I would say it doesn't express the intention of the flag which is actually "should be applied".
Would be good to find some better name. May be setUseDefaultThemeor shouldUseDefaultTheme (though I don't like should in method names).

Done.

flow-server/src/main/java/com/vaadin/flow/component/page/LoadingIndicatorConfiguration.java, line 88 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…
isDefaultThemeApplied

Not sure whether the meaning of the flag is different here. But most likely it's the same issue with the naming here.

Done.

Comments from Reviewable

@denis-anisimov
Copy link
Contributor

:lgtm:

Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 9 issues

  • CRITICAL 5 critical
  • MAJOR 4 major

Watch the comments in this conversation to review them.

4 extra issues

Note: 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:

  1. MAJOR InitialPageSettings.java#L1: Class com.vaadin.flow.server.InitialPageSettings defines non-transient non-serializable instance field elements rule
  2. MAJOR InitialPageSettings.java#L1: Class com.vaadin.flow.server.InitialPageSettings defines non-transient non-serializable instance field request rule
  3. MAJOR InitialPageSettings.java#L56: Make "request" transient or serializable. rule
  4. MAJOR InitialPageSettings.java#L66: Make "elements" transient or serializable. rule

@pleku pleku merged commit 6f2dd91 into master May 29, 2018
@pleku pleku deleted the loading-indicator branch May 29, 2018 07:59
pleku added a commit to vaadin/flow-and-components-documentation that referenced this pull request May 29, 2018
@pleku
Update loading indicator documentation
2a2826e 
Needs vaadin/flow#4174 to be merged before it compiles.
@denis-anisimov denis-anisimov added this to the 1.0.0.rc1 milestone May 30, 2018
pleku added a commit to vaadin/flow-and-components-documentation that referenced this pull request Jun 14, 2018
@pleku
Update loading indicator documentation
ea4c2bc 
Needs vaadin/flow#4174 to be merged before it compiles.
pleku added a commit to vaadin/flow-and-components-documentation that referenced this pull request Jun 26, 2018
@pleku
Update loading indicator documentation
969867b 
Needs vaadin/flow#4174 to be merged before it compiles.
denis-anisimov pushed a commit to vaadin/flow-and-components-documentation that referenced this pull request Jun 26, 2018
@pleku
Update loading indicator documentation (#188)
6bd9275 
* Update loading indicator documentation

Needs vaadin/flow#4174 to be merged before it compiles.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaadin-bot vaadin-bot vaadin-bot left review comments

Assignees
No one assigned
Labels
breaking change theming
Projects
None yet
Milestone
1.0.0.rc1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pleku @denis-anisimov @vaadin-bot