-
Notifications
You must be signed in to change notification settings - Fork 772
feat(ssr): enhance support for Universal and SSR with stylesheets #567
Conversation
887e063
to
1e36be6
Compare
6a371f0
to
ee9d608
Compare
Example output for the following HTML markup: HTML Markup<div class="night-theme">
<div fxLayout="row" fxLayout.xs="column" style="height:500px" ngxSplit="row">
<div fxFlex="30%" ngxSplitArea class="c1r1">
<div class="c1r1_header">Column #1 - Row #1</div>
<ul>
<li>2 Columns: 30% + 70%</li>
<li>2nd Column: 2 rows</li>
<li>2nd Column: 50% + 50%</li>
</ul>
</div>
<div class="handle handle-row" ngxSplitHandle>
<i class="material-icons"></i>
</div>
<div fxFlex.xs="70%" fxFlex.gt-md="50%" fxFlex.lg="60%" ngxSplitArea>
<div fxLayout="column" fxFlexFill ngxSplit="column">
<div fxFlex="50%" ngxSplitArea class="c2r1_body">
<div class="c2r1_header">Column #2 - Row #1</div>
<h1>Layout Dashboard</h1>
<p>
Demonstrate use of ngxSplit with the Flex-Layout API
and flexbox css layouts.
<br/><br/>
Haxx0r ipsum cd ctl-c Starcraft concurrently salt unix baz class bar linux
January 1, 1970 syn for mutex daemon todo mountain dew recursively. Mainframe
wannabee machine code hack the mainframe do void python bin big-endian break
tcp ddos emacs public frack.Over clock headers data private *.* pwned
fork script kiddies.
</p>
</div>
<div class="handle handle-column" ngxSplitHandle>
<i class="material-icons"></i>
</div>
<div fxFlex="50%" ngxSplitArea class="c2r2">
<div class="c2r2_header">Column #2 - Row #2</div>
<ul>
<li>List Item #1</li>
<li>List Item #2</li>
<li>List Item #3</li>
</ul>
</div>
</div>
</div>
</div>
</div> Generated CSS@media all {
.flex-layout-0 {
height: 100%;
min-height: 100%;
min-width: 100%;
width: 100%;
-webkit-flex-direction: column;
box-sizing: border-box;
display: flex;
flex-direction: column;
}
.flex-layout-1 {
-webkit-flex-direction: row;
box-sizing: border-box;
display: flex;
flex-direction: row;
}
.flex-layout-2 {
-webkit-flex: 1 1 0.000000001px;
box-sizing: border-box;
flex: 1 1 0.000000001px;
max-width: 30%;
-webkit-flex-basis: 100%;
-webkit-flex-grow: 1;
-webkit-flex-shrink: 1;
flex-basis: 100%;
flex-grow: 1;
flex-shrink: 1;
}
.flex-layout-3 {
-webkit-flex: 1 1 0.000000001px;
box-sizing: border-box;
flex: 1 1 0.000000001px;
}
.flex-layout-4 {
-webkit-flex: 1 1 0.000000001px;
box-sizing: border-box;
flex: 1 1 0.000000001px;
max-height: 50%;
-webkit-flex-basis: 100%;
-webkit-flex-grow: 1;
-webkit-flex-shrink: 1;
flex-basis: 100%;
flex-grow: 1;
flex-shrink: 1;
}
.flex-layout-5 {
-webkit-flex: 1 1 0.000000001px;
box-sizing: border-box;
flex: 1 1 0.000000001px;
max-height: 50%;
-webkit-flex-basis: 100%;
-webkit-flex-grow: 1;
-webkit-flex-shrink: 1;
flex-basis: 100%;
flex-grow: 1;
flex-shrink: 1;
}
}
@media (min-width: 1280px) and (max-width: 1919px) {
.flex-layout-1 {
-webkit-flex-direction: row;
box-sizing: border-box;
display: flex;
flex-direction: row;
}
.flex-layout-3 {
-webkit-flex-basis: 100%;
-webkit-flex-grow: 1;
-webkit-flex-shrink: 1;
box-sizing: border-box;
flex-basis: 100%;
flex-grow: 1;
flex-shrink: 1;
max-width: 60%;
}
}
@media (max-width: 1279px) {
.flex-layout-1 {
-webkit-flex-direction: row;
box-sizing: border-box;
display: flex;
flex-direction: row;
}
.flex-layout-3 {
-webkit-flex: 1 1 0.000000001px;
box-sizing: border-box;
flex: 1 1 0.000000001px;
}
}
@media (min-width: 1280px) {
.flex-layout-1 {
-webkit-flex-direction: row;
box-sizing: border-box;
display: flex;
flex-direction: row;
}
.flex-layout-3 {
-webkit-flex-basis: 100%;
-webkit-flex-grow: 1;
-webkit-flex-shrink: 1;
box-sizing: border-box;
flex-basis: 100%;
flex-grow: 1;
flex-shrink: 1;
max-width: 50%;
}
}
@media (min-width: 960px) and (max-width: 1279px) {
.flex-layout-1 {
-webkit-flex-direction: row;
box-sizing: border-box;
display: flex;
flex-direction: row;
}
.flex-layout-3 {
-webkit-flex: 1 1 0.000000001px;
box-sizing: border-box;
flex: 1 1 0.000000001px;
}
}
@media (min-width: 0px) and (max-width: 599px) {
.flex-layout-1 {
-webkit-flex-direction: column;
box-sizing: border-box;
display: flex;
flex-direction: column;
}
.flex-layout-2 {
-webkit-flex-basis: 100%;
-webkit-flex-grow: 1;
-webkit-flex-shrink: 1;
box-sizing: border-box;
flex-basis: 100%;
flex-grow: 1;
flex-shrink: 1;
max-height: 30%;
}
.flex-layout-3 {
-webkit-flex: 1 1 0.000000001px;
box-sizing: border-box;
flex: 1 1 0.000000001px;
max-height: 70%;
-webkit-flex-basis: 100%;
-webkit-flex-grow: 1;
-webkit-flex-shrink: 1;
flex-basis: 100%;
flex-grow: 1;
flex-shrink: 1;
}
} Minified CSS @media all{.flex-layout-0{height:100%;min-height:100%;min-width:100%;width:100%;-webkit-flex-direction:column;box-sizing:border-box;display:flex;flex-direction:column;}.flex-layout-1{-webkit-flex-direction:row;box-sizing:border-box;display:flex;flex-direction:row;}.flex-layout-2{-webkit-flex:1 1 100%;box-sizing:border-box;flex:1 1 100%;max-width:30%;}.flex-layout-3{-webkit-flex:1 1 0.000000001px;box-sizing:border-box;flex:1 1 0.000000001px;}.flex-layout-4{-webkit-flex:1 1 100%;box-sizing:border-box;flex:1 1 100%;max-height:50%;}.flex-layout-5{-webkit-flex:1 1 100%;box-sizing:border-box;flex:1 1 100%;max-height:50%;}}@media (min-width: 1280px) and (max-width: 1919px){.flex-layout-1{-webkit-flex-direction:row;box-sizing:border-box;display:flex;flex-direction:row;}.flex-layout-3{-webkit-flex:1 1 100%;box-sizing:border-box;flex:1 1 100%;max-width:60%;}}@media (min-width: 1280px){.flex-layout-1{-webkit-flex-direction:row;box-sizing:border-box;display:flex;flex-direction:row;}.flex-layout-3{-webkit-flex:1 1 100%;box-sizing:border-box;flex:1 1 100%;max-width:50%;}}@media (min-width: 0px) and (max-width: 599px){.flex-layout-1{-webkit-flex-direction:column;box-sizing:border-box;display:flex;flex-direction:column;}.flex-layout-2{-webkit-flex:1 1 100%;box-sizing:border-box;flex:1 1 100%;max-height:30%;}.flex-layout-3{-webkit-flex:1 1 100%;box-sizing:border-box;flex:1 1 100%;max-height:70%;}} Runtime DOM values<body>
<responsive-app _nghost-c0="" ng-version="5.2.0">
<div _ngcontent-c0="" class="night-theme">
<div _ngcontent-c0="" class="ngx-split flex-layout-1" fxlayout="row" fxlayout.xs="column" ngxsplit="row" style="height:500px">
<div _ngcontent-c0="" class="c1r1 flex-layout-2" fxflex="30%" ngxsplitarea="" style="overflow: auto;">
<div _ngcontent-c0="" class="c1r1_header">Column #1 - Row #1</div>
<ul _ngcontent-c0="">
<li _ngcontent-c0="">2 Columns: 30% + 70%</li>
<li _ngcontent-c0="">2nd Column: 2 rows</li>
<li _ngcontent-c0="">2nd Column: 50% + 50%</li>
</ul>
</div>
<div _ngcontent-c0="" class="handle handle-row ngx-split-handle" ngxsplithandle="" title="Drag to resize">
<i _ngcontent-c0="" class="material-icons"></i>
</div>
<div _ngcontent-c0="" fxflex.gt-md="50%" fxflex.lg="60%" fxflex.xs="70%" ngxsplitarea="" style="overflow: auto;" class="flex-layout-3">
<div _ngcontent-c0="" class="ngx-split flex-layout-0" fxflexfill="" fxlayout="column" ngxsplit="column">
<div _ngcontent-c0="" class="c2r1_body flex-layout-4" fxflex="50%" ngxsplitarea="" style="overflow: auto;">
<div _ngcontent-c0="" class="c2r1_header">Column #2 - Row #1</div>
<h1 _ngcontent-c0="">Layout Dashboard</h1>
<p _ngcontent-c0="">
Demonstrate use of ngxSplit with the Flex-Layout API
and flexbox css layouts.
<br _ngcontent-c0=""><br _ngcontent-c0="">
Haxx0r ipsum cd ctl-c Starcraft concurrently salt unix baz class bar linux
January 1, 1970 syn for mutex daemon todo mountain dew recursively. Mainframe
wannabee machine code hack the mainframe do void python bin big-endian break
tcp ddos emacs public frack.Over clock headers data private *.* pwned
fork script kiddies.
</p>
</div>
<div _ngcontent-c0="" class="handle handle-column ngx-split-handle" ngxsplithandle="" title="Drag to resize">
<i _ngcontent-c0="" class="material-icons"></i>
</div>
<div _ngcontent-c0="" class="c2r2 flex-layout-5" fxflex="50%" ngxsplitarea="" style="overflow: auto;">
<div _ngcontent-c0="" class="c2r2_header">Column #2 - Row #2</div>
<ul _ngcontent-c0="">
<li _ngcontent-c0="">List Item #1</li>
<li _ngcontent-c0="">List Item #2</li>
<li _ngcontent-c0="">List Item #3</li>
</ul>
</div>
</div>
</div>
</div>
</div>
</responsive-app>
</body> |
305ab8f
to
193fdab
Compare
Note: this needs a testing mode where we can change the PLATFORM_ID flag from browser mode to server mode and confirm all tests still pass. @devversion any thoughts on this? |
193fdab
to
fd38110
Compare
28affd8
to
6ebd21f
Compare
a7a8a51
to
4d9e74f
Compare
The platform-server part and associated tests looks good to me(I didn't go through the actual styles logic). Just need a small clarification. (In the future it would help if the PRs are collection of smaller PRs/commits) |
@Optional() @Inject(SERVER_TOKEN) protected _serverModuleLoaded: boolean) { | ||
super(_monitor, _elRef, _styler); | ||
this._cacheInput('src', _elRef.nativeElement.getAttribute('src') || ''); | ||
if (isPlatformServer(this._platformId) && this._serverModuleLoaded) { |
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.
Can you explain a bit on how the serverModuleLoaded flag is used?
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.
The flag is to determine if the FlexLayoutServerModule
is included (since it's the only module that provides that token).
Loading the server module is optional -- if a user doesn't load it, it means they want the inline styles as a fallback (it also prevents this from being a breaking change, since it's totally opt-in).
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.
Will this get into the docs?
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.
Absolutely. Docs will be in a follow-up PR once this has all been finalized
@@ -204,7 +205,7 @@ export const customMatchers: jasmine.CustomMatcherFactories = { | |||
* specified DOM element. | |||
*/ | |||
function buildCompareStyleFunction(inlineOnly = true) { | |||
return function (actual: any, styles: { [k: string]: string } | string) { | |||
return function (actual: any, styles: { [k: string]: string } | string, styler: StyleService) { |
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.
Is it really necessary to change this signature? It adds a significant amount of test boilerplate.
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.
Discussed offline, unfortunately very necessary 👎
src/lib/media-query/match-media.ts
Outdated
} | ||
|
||
removeListener(_: MediaQueryListListener) { | ||
} |
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.
Add comment in function body here like
// Don't need to remove listeners in the server environment.
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.
Added
src/lib/media-query/match-media.ts
Outdated
// Function with Window's MediaQueryList argument | ||
(mql: MediaQueryList): void; | ||
} | ||
export class ServerMediaQueryList implements MediaQueryList { |
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.
This should be in a separate file
(generally all server-specific stuff should be in separate files so that you can better ensure you're only getting what you need on the client)
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.
Added to a separate file with a separate implementation of MatchMedia
for the server as suggested below
src/lib/media-query/match-media.ts
Outdated
@@ -51,19 +92,36 @@ export interface MediaQueryList { | |||
*/ | |||
@Injectable() | |||
export class MatchMedia { | |||
protected _registry: Map<string, MediaQueryList>; | |||
protected _registry: Map<string, MediaQueryList|ServerMediaQueryList>; |
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.
Instead of using a union type, it would be better to use an interface and an InjectionToken
. When using a union type, you may end up getting the server-side code on the client
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.
Added, great suggestion!
src/lib/server/server-provider.ts
Outdated
* * Adds a unique class to each element and stores it | ||
* in a shared classMap for later reuse | ||
*/ | ||
function formatStyle(stylesheet: Map<HTMLElement, Map<string, string|number>>, |
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.
Something like generateCss
?
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.
Fixed
*/ | ||
import {InjectionToken} from '@angular/core'; | ||
|
||
export const SERVER_TOKEN = new InjectionToken<boolean>('FlexLayoutServerLoaded'); |
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.
Description?
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.
Added
src/lib/utils/styling/styler.ts
Outdated
import {SERVER_TOKEN} from './server-token'; | ||
|
||
@Injectable() | ||
export class StyleService { |
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.
Class description?
On naming: my rule is to avoid the term "Service" in class names because it doesn't tell you anything about what the class actually does. You'll notice in material we have things like ScrollDispatcher
, FocusMonitor
, InteractivityChecker
, etc. instead of ScrollService
, FocusService
, InteracitivityService
. Adding a more specific noun makes the name much more meaningful.
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.
I vaguely recall @ThomasBurleson saying the same. The original name was StyleUtils
, I'll change it back.
src/lib/utils/styling/styler.ts
Outdated
|
||
/** | ||
* Determine the inline or inherited CSS style | ||
* @TODO(CaerusKaru): platform-server has no implementation for getComputedStyle |
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.
What's the action on this TODO? The whole idea of getComputedStyle
inherently can't work on the server (without being driven by an entire browser rendering engine)
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.
Changed to NOTE
src/lib/utils/styling/styler.ts
Outdated
value = getComputedStyle(element).getPropertyValue(styleName); | ||
} | ||
} else { | ||
if (this._serverModuleLoaded) { |
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.
There are a lot of places throughout the PR where you're explicitly performing a check for browser vs. server and performing a different action for each. In general it would be better to delegate to some injectable with a common interface that uses a different implementation on each platform.
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.
Now that the MatchMedia
has been changed (see above), the only three places this still happens are the following: ImgSrc, ShowHide, and Styler. In each case, the check only changes about two lines of code in very critical ways. I think it would duplicate a lot of code to change and as it stands now doesn't benefit much to factor out.
src/lib/utils/styling/styler.ts
Outdated
* Definition of a css style. Either a property name (e.g. "flex-basis") or an object | ||
* map of property name and value (e.g. {display: 'none', flex-order: 5}) | ||
*/ | ||
export type StyleDefinition = string | { [property: string]: string | number | null }; |
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.
Why do either an object literal or string instead of just always doing an object literal?
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.
This is legacy code, removed. (The idea was that when calling a function, you could call it with string and value individually instead of providing an object)
5d67fc1
to
176958e
Compare
@jelbourn Comments addressed, PTAL |
import { | ||
NgZone, | ||
PLATFORM_ID, | ||
InjectionToken, // tslint:disable-line:no-unused-variable |
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.
You can avoid suppressing the lint error by explicitly using InjectionToken
in the place where it is inferred
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.
Fixed (this is a real pain by the way, is it a bug in tslint
)?
* Create a version of MatchMedia compatible with the current | ||
* platform | ||
*/ | ||
export function MEDIA_QUERY_LIST_FACTORY(platformId: Object, |
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.
Instead of having one provider that returns either service, there should just be two different providers. The appropriate provider is then included in each of the browser / server module.
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.
Fixed
*/ | ||
export class ServerMediaQueryList implements MediaQueryList { | ||
private _isActive = false; | ||
private _listeners: Array<MediaQueryListListener> = []; |
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.
MediaQueryListListener[]
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.
Fixed (and fixed the source for this)
|
||
/** | ||
* Notify all listeners that 'matches === TRUE' | ||
*/ |
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.
You can collapse short comment to a single line:
/** Notify all listeners that 'matches === TRUE' */
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.
Fixed
src/lib/server/server-provider.ts
Outdated
ServerMatchMedia | ||
} from '@angular/flex-layout'; | ||
|
||
let UNIQUE_CLASS = 0; |
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.
UNIQUE_CLASS
shouldn't be caps case because it's not constant. I typically call this nextId
in material (example)
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.
Fixed
APP_BOOTSTRAP_LISTENER, | ||
PLATFORM_ID, | ||
InjectionToken, // tslint:disable-line:no-unused-variable | ||
ComponentRef, // tslint:disable-line:no-unused-variable |
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.
You can avoid the suppression by explicitly using them in the code where they are now inferred.
57ba228
to
fa7839c
Compare
@jelbourn Comments addressed again, PTAL |
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.
No further comments from me, I'll leave it to Thomas from here
a936361
to
e9c77b0
Compare
* Add `StyleService` class to manage application and retrieval of styles from elements in a platform-agnostic manner * Add virtual stylesheet to store server styles, which applies default styles when breakpoint overrides are not present * While not in the browser (ssr), intercept all style calls and reroute them to the virtual stylesheet. * For server-side rendering, add a new type of MediaQueryList similar to the MockMediaQueryList to support manual activation/deactivation of breakpoints * Add jasmine testing mode for SSR * Add FlexLayoutServerModule to invoke SSR styling * Remove unnecessary Renderer references and replace them with DOM APIs * Add whitespace debugging mode for server styles Fixes #373. > See [Design Doc](https://docs.google.com/document/d/1fg04ihw42dJJHGd6fugdiBe39iJot8aErhiE7CjwfmQ/edit#)
3263489
to
acf7e2f
Compare
Merged with SHA cf5266a |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
StyleService
class to manage application and retrieval of styles from elements in a platform-agnostic mannerFixes #373.