-
Notifications
You must be signed in to change notification settings - Fork 161
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
Flow initial pwa #4334
Flow initial pwa #4334
Changes from 24 commits
74ccbf0
110a643
b03bdb0
1953c59
3538cbf
8908c3b
fc2a561
f52538a
5b0b1cc
10c19f1
97d580c
efeb58e
e19249a
b942b51
45f9931
96451c3
11454ad
3c4a794
e0ab2da
067fb12
2185806
a5ab9fe
c8b77e2
a73ebe4
9a44794
f7ad12c
d5d926f
ec3e500
eb57b39
3a74bd5
d8adc3f
ca2d2f4
6c4b20a
15a789f
663cd54
c71e058
682c3ad
0d67035
efb7050
b2bd0cd
3612a12
38c0460
e1fe75d
b4c69d0
ff85742
34eeaa7
6af74cd
4da44a9
689730a
a55390a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
/* | ||
* Copyright 2000-2017 Vaadin Ltd. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not | ||
* use this file except in compliance with the License. You may obtain a copy of | ||
* the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
* License for the specific language governing permissions and limitations under | ||
* the License. | ||
*/ | ||
package com.vaadin.flow.server; | ||
|
||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
/** | ||
* Defines application PWA properties. | ||
* | ||
* Annotation required in order activate automatic pwa injecting. | ||
* | ||
* Only 1 annotation for application is supported. | ||
* | ||
* Annotation must be placed to master layout. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will confuse people with multiple root parent layouts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One is enough, it's used throughout the whole application. I agree that it is a bit confusing. In fact, at the moment (with current implementation) you could add the PWA annotation within any route or parent layout, so the comment isn't accurate either. Any ideas on how to make it less confusing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's another problem also. Now if you for some reason (Forgetting a PWA when copying or something) have multiple PWA annotations then one of them will be selected and you can't really know which one, might even be a different one each time if you are unlucky. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, we should just throw exception if multiple PWA annotations are in place. I though I had it already, but seems that I removed later on. I'll fix that one. |
||
* | ||
* Application annotated with enabled PWA will add following capabilities | ||
* to flow application: | ||
* | ||
* - handle manifest.json | ||
* - handle sw.js (service worker), | ||
* which will handle simple offline fallback and file caching | ||
* - handle default (static) offline html page | ||
* - handle different versions (sizes) of given logo | ||
* - inject needed tags to header | ||
* | ||
* Any of the handled resources can be explicitly overridden with static | ||
* file in public resources. | ||
* | ||
* For example, if "manifest.json" is available in webapp root folder it | ||
* will be served instead of generated manifest.json. Same applies for | ||
* service worker and generated icons. | ||
*/ | ||
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.TYPE) | ||
public @interface PWA { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a lot of methods (attributes) in this annotation. Looks inconvenient to me. I'm not sure exactly how it may be improved. I would change in this case
Then there will be an additional responsibility to instantiate the provided class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aye, there are quite a lot of those properties. But I like the developer experience of having all that in that annotation: that way adding values and/or checking default values is supported directly with IDE. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
|
||
/** | ||
* Path to static offline html file. | ||
* | ||
* Defaults to (relative) "offline.html" | ||
* with default configuration that is webapp/offline.html | ||
* | ||
* If offline file is not found, falls back to default offline page | ||
* | ||
* @return Path to static offline html file | ||
*/ | ||
String offlinePath() default PwaConfiguration.DEFAULT_OFFLINE_PATH; | ||
|
||
/** | ||
* Path to manifest file. | ||
* | ||
* Defaults to (relative) "manifest.json" | ||
* with default configuration that is webapp/manifest.json | ||
* | ||
* @return Path to manifest file | ||
*/ | ||
String manifestPath() default PwaConfiguration.DEFAULT_PATH; | ||
|
||
/** | ||
* Path to logo. | ||
* | ||
* Defaults to (relative) "icons/logo.png" | ||
* | ||
* with default configuration that is webapp/manifest.json | ||
* | ||
* If the logo -file is not found, falls back to default logo. | ||
* | ||
* Logo is also used to create different sizes of logo. | ||
* | ||
* @return path to logo | ||
*/ | ||
String logoPath() default PwaConfiguration.DEFAULT_LOGO; | ||
|
||
/** | ||
* Name of the application. | ||
* | ||
* @return Name of the application | ||
*/ | ||
String name(); | ||
|
||
/** | ||
* Short name for application. | ||
* | ||
* Maximum of 12 characters. | ||
* | ||
* @return Short name for application | ||
*/ | ||
String shortName(); | ||
|
||
/** | ||
* Description of application. | ||
* | ||
* @return Description of application | ||
*/ | ||
String description() default ""; | ||
|
||
/** | ||
* Theme color of application. | ||
* | ||
* The theme color sets the color of the tool bar, and in the task switcher. | ||
* | ||
* @return Theme color of application | ||
*/ | ||
String themeColor() default PwaConfiguration.DEFAULT_THEME_COLOR; | ||
|
||
/** | ||
* Background color of application. | ||
* | ||
* The background_color property is used on the splash screen when the | ||
* application is first launched. | ||
* | ||
* @return Background color of application | ||
*/ | ||
String backgroundColor() default PwaConfiguration.DEFAULT_BACKGROUND_COLOR; | ||
|
||
/** | ||
* Defines the developers’ preferred display mode for the website. | ||
* | ||
* Possible values: | ||
* fullscreen, standalone, minimal-ui, browser | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered making this an enum? |
||
* | ||
* @return display mode of application | ||
*/ | ||
String display() default PwaConfiguration.DEFAULT_DISPLAY; | ||
|
||
/** | ||
* Offline resources to be cached with service worker. | ||
* | ||
* @return Offline resources to be cached | ||
*/ | ||
String[] offlineResources() default {}; | ||
|
||
/** | ||
* Is PWA injecting enabled. | ||
* | ||
* @return Is PWA injecting enabled | ||
*/ | ||
boolean enabled() default true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a use case when someone adds the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No there's not. That was leftover from old code. Removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to publish my review in time. Sorry, new UI of GitHub review. |
||
} |
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.
VaadinService.getCurrent()
Please don't do this.
Artur made an effort to avoid
getCurrent()
methods call whenever it's possible.Please pass
BootstrapContext context
as a method parameter and use it to getVaadinSession
and a service from it. There are examples in the methods of this class.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.
Done by Mikko.