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

Create a AbstractTheme class for controlling theme #3196

Closed
caalador opened this issue Dec 21, 2017 · 7 comments
Closed

Create a AbstractTheme class for controlling theme #3196

caalador opened this issue Dec 21, 2017 · 7 comments
Assignees
Milestone

Comments

@caalador
Copy link
Contributor

caalador commented Dec 21, 2017

AbstractTheme should contain abstract methods that
will be used to populate any extra things wanted to the
initial Bootstrap page (like with PageConfigurator, @Inline or BootstrapListener)

AbstractTheme should also contain import filtering with simple
pattern matching to rewrite html imports as needed. e.g. "/src/" -> "/theme/valo/"
If the file exists (either directly through ServletContext or in a WebJar), then the rewritten name is used.
(Note! Might require caching logic to reduce I/O overhead, Might cause extra complexity for checking whether a file exists if it has been moved into a bundle)

@heruan
Copy link
Member

heruan commented Dec 21, 2017

The dependency-filter/rewrite approach kinda scares me, but I understand it must have been discussed thoroughly if that's the final choice.

Just out of curiosity, why not having the @Tag/@HtmlImport annotation look if there's a theme-for available for that element (matching the selected theme) and import that file?

@Legioth
Copy link
Member

Legioth commented Dec 21, 2017

Some aspects from our discussions:

  1. Rewriting should be quite safe since the rewrite result is only used if the file actually exists. The rewritten names should be quite specific (e.g. vaadin-button/theme/valo/vaadin-button.html), so the risk of conflicts is minimal.
  2. With hardcoded mappings for each theme, the Java part would have to be updated every time the theme is updated. This would cause additional maintenance overhead and increase the risk of human error.
  3. Theme loading support is quite specific to Vaadin's own Web Components and any third party implementations that choose to follow the same pattern. @HtmlImport and @Tag are generic features intended for any Web Component and should thus not directly know anything about themes.
  4. We foresee composite themes that supplement e.g. Valo with similar theming for additional elements while still using the built-in Valo theming when available. Supporting this with a declarative approach would either require lots of maintenance or complex-ish rules for how to mix declarations. With an imperative approach, the rewriting logic of one theme can instead have arbitrary logic for delegating to the rewriting logic of other themes.

@heruan
Copy link
Member

heruan commented Dec 21, 2017

Thank you @Legioth for exposing these aspects, they seem totally reasonable considering the specificity of Vaadin's elements styling. My main concerns at first were related to bundling and JRebel-like hot deployment tools, but then it's up to them to handle the process correctly.

BTW with my (totally hasty) idea, I didn't mean to add theming support to @Tag or @HtmlImport, just importing the theme file at runtime instead of rewriting; something like:

  1. a component is annotated with @Tag("valo-button");
  2. if we have a theme-for="valo-button" in our classpath, part of the current selected theme (e.g. ThemeResolver.getCurrentTheme() == "valo"/"material", import it.

Then the user may also be able to choose/change the theme while using the app.

@Legioth
Copy link
Member

Legioth commented Dec 21, 2017

Bundling needs some special attention regardless of whether the definition of what to import is determined based on a generic rewrite rule or a hardcoded mapping. The plan is to integrate this into our Maven plugin (and preferably do that in a way that is also reusable for other build tool integrations). I now realized that one implication of this is that the rewriting logic should probably not be directly based on DependencyFilter, but rather a more specific abstraction that is intended to be used both by the build tool and the runtime rewriting mechanism (ping @caalador).

I don't think tools like JRebel would cause any problems here since the rewrite would still be applied again every time a @Htmlmport is encountered (as long as e.g. caching of file system lookups are also cleared during hot redeploy).

Changing the theme of an already loaded app is not supported on the Web Component level. The page always has to be reloaded in the browser to avoid conflicts with a previously loaded theme.

@caalador
Copy link
Contributor Author

reworded to 'import filtering with simple pattern matching'

@Legioth
Copy link
Member

Legioth commented Dec 21, 2017

reworded to 'import filtering with simple pattern matching'

I don't think it even needs to be "simple". It can still be arbitrary string handling logic. When bundling, the callback would just be run once for each available input file.

@pleku
Copy link
Contributor

pleku commented Jan 8, 2018

The bundling part, e.g. using the pattern to figure out what sources need to be in the bundles, is excluded from this issue.

@pleku pleku closed this as completed Jan 10, 2018
@pleku pleku added this to the 1.0.0.alpha17 milestone Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants