-
Notifications
You must be signed in to change notification settings - Fork 772
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
rfc: new layout system #1185
base: master
Are you sure you want to change the base?
rfc: new layout system #1185
Conversation
6edd87d
to
5d99b4a
Compare
f8b022e
to
73323b4
Compare
Is there any estimated release date for this ? |
I don't believe so. |
I love the 70% bundle size reduction! I'm curious how much of that is JS vs CSS? Would this be valid? <bp name="gt-xs" layout="row" gap="25px"> Or do you need to do this? <bp name="gt-xs" layout="row">
<bp name="gt-xs" gap="25px">
|
@Splaktar
|
Thank you for the quick response! I'm just starting to dig into the full |
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.
Sorry for all of the questions, but this is what jumped out at me on my first look over this approach.
I like the general direction!
<bp tag="xs" flexAlign="end"></bp> | ||
<bp tag="md" flexAlign="center"></bp> |
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 PR description uses name="xs"
here. It looks like the rest of the code uses tag="xs"
.
I'm not sure which one is clearer, probably name
?
I guess the formal name for "the stuff inside the media query CSS at-rule" is a "media query list". But I don't think that we support multiple queries per <bp>
. Perhaps it should just be "query="xs"
?
</div> | ||
``` | ||
|
||
Here, `ngl` stands for "Angular (ng) Layout (l)". The `bp` elements are optional if |
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.
Was ngl
selected just because it's shorter than ngLayout
?
|
||
Here, `ngl` stands for "Angular (ng) Layout (l)". The `bp` elements are optional if | ||
you don't plan to use breakpoints in your template. Conceptually, it's cleaner than | ||
cramming all of the breakpoints on one root element, and the `bp` elements don't |
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.
bp
is short, but I think that it lacks a little bit of expressiveness. It won't be readable to people who aren't familiar with this library's APIs.
Is <breakpoint
overly verbose? It seems a lot more readable and expressive.
<div ngLayout flexAlign="start">
<breakpoint tag="xs" flexAlign="end"></breakpoint>
<breakpoint tag="md" flexAlign="center"></breakpoint>
</div>
* tagging an HTML element as part of the layout system, and then | ||
* coordinating all updates with GrandCentral. | ||
*/ | ||
@Directive({selector: `[ngl]`}) |
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 seems like a structural directive which should have a *
in front of it (when used in a template), since it can change the structure of the DOM (especially when a directive like order="2"
is used).
But then that introduces the limitation of one structural directive per host element.
Have you considered this approach? Were issues uncovered?
* for each of the media states. | ||
*/ | ||
@Injectable({providedIn: 'root'}) | ||
export class GrandCentral { |
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.
GrandCentral
is semi-obvious for Americans who live on the East Coast, but I'm not sure it's as universally well-known as we might want. Would something like Orchestrator
or Conductor
(I think that this could have minor collisions with other meanings of the word) be more widely understood?
/** The shortcut name for the breakpoint, e.g. 'xs' */ | ||
name: string; | ||
/** The mediaQuery for the breakpoint, e.g. 'screen and (max-width: 500px)' */ | ||
media: string; |
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 think that there's a good case for calling this query
or mediaQuery
.
* An internal-facing injection token to consolidate all registered | ||
* breakpoints for use in the application. | ||
*/ | ||
export const BPS = new InjectionToken<Breakpoint[]>('Angular Layout Condensed Breakpoints', { |
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.
It's a short token, but it's not very expressive.
* associated name, builder, cache, and dependencies on other builder input | ||
* values. | ||
*/ | ||
export abstract class Tag { |
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.
In this implementation, Tag
s are attributes? I think that this can be easily confused with HTML tags.
Does this implementation reduce the amount of layout thrashing compared to what the existing implementation does? The performance issues with the use of inline styles in the existing implementation is discussed here: #1299 |
@fxck Please consider moving your comment to the discussion thread, since your comment has no bearing on this PR. @samal-rasmussen Last I remember, this does not incorporate a class-based approach for a few reasons. 1) was to semi-maintain parity with the existing toolset, which prioritizes style CSS rank vs performance, and 2) there are certain features that simply can't be handled with a class-based approach. This does not mean we won't add that functionality later, or in the current version of the library; it's distinct from this proposal, which is more about how to structure the directives themselves in the markup. |
@CaerusKaru are there any plans for an automatic upgrade using Also is there a link to the discussion thread? I could find anything here. |
@epelc I would love to have some |
One thing is special about the |
Angular Layout Condensed
Introduction
In its original form, Angular Flex Layout was a series of twenty or so directives. Each directive was meant to correspond to a unique type of style framework (e.g. the Flex directive works to make applying
flex
CSS styles easier). The selectors for each directive were extended to include responsive breakpoints to easily add and remove stylings based on the current active media query of the page. This gave developers an incredible tool in designing responsive webpages.However, this approach led to a lot of bloat, as the overhead in shipping twenty-plus Angular directives, and the core utilities to power them, took its toll. Not only that, but it became harder and harder to write directives that would create conflicting styles. Finally, the performance hit of multiple directives instantiated on a single element was also non-trivial, despite the addition of a style caching mechanism.
Therefore, a new approach was developed, favoring a single, unified directive and centralized media processing service. This allows us to completely decouple the style generation utilities from the media responsive engine, while simultaneously allowing developers to seamlessly extend both.
A New Syntax
Previously, the way to describe a template in Angular Flex Layout was as follows:
Straight away, you can see there's now a lot of clutter. The intention of the stylings are lost under the weight of the breakpoints. Ordering doesn't matter, and it's easy to create conflicts or to lose sight of certain values.
The new syntax in Angular Layout Condensed (for the same template) is as follows:
Here, the
ngl
(Angular Layout) directive is the unified directive that is responsible for registering values to the centralized media processing service. Thebp
directives are responsible for simply acting as wrappers for capturing the values for those specific breakpoints, for consumption by the host directive.The benefits of the new approach make the overall template simpler and easier to reason about. Determining expectations vs reality is also much more straightforward.
Finally, the ultimate benefit of the new approach: a 70% size reduction compared to the previous version, coming out to ~25KB minified, with room for improvement.
Other details are omitted from this design, like the ease of extending the interface to provide new breakpoints and style generation providers. Needless to say, that experience is also greatly improved.