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

rfc: new layout system #1185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

rfc: new layout system #1185

wants to merge 1 commit into from

Conversation

CaerusKaru
Copy link
Member

@CaerusKaru CaerusKaru commented Feb 2, 2020

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:

<div fxLayout.xs="column" fxLayout.gt-xs="row" fxLayoutGap="10px" fxLayoutGap.md="25px">
  <div fxFlex>Part 1</div>
  <div fxFlex>Part 2</div>
  <div fxFlex>Part 3</div>
</div>

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:

<div ngl gap="10px">
  <bp name="xs" layout="column">
  <bp name="gt-xs" layout="row">
  <bp name="md" gap="25px">
  <div ngl flex>Part 1</div>
  <div ngl flex>Part 2</div>
  <div ngl flex>Part 3</div>
</div

Here, the ngl (Angular Layout) directive is the unified directive that is responsible for registering values to the centralized media processing service. The bp 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.

@CaerusKaru CaerusKaru added the discussion Further discussion with the team is needed before proceeding label Feb 2, 2020
@CaerusKaru CaerusKaru force-pushed the adam/one branch 4 times, most recently from 6edd87d to 5d99b4a Compare February 10, 2020 05:25
@CaerusKaru
Copy link
Member Author

Ref: #1141, #1130, #1161, #1035, #501

@CaerusKaru CaerusKaru force-pushed the adam/one branch 2 times, most recently from f8b022e to 73323b4 Compare February 11, 2020 06:47
@erkanmaras
Copy link

Is there any estimated release date for this ?

@CaerusKaru CaerusKaru mentioned this pull request May 7, 2020
@CaerusKaru CaerusKaru added the rfc label May 10, 2020
@Splaktar
Copy link
Member

Is there any estimated release date for this ?

I don't believe so.

@Splaktar
Copy link
Member

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">
  1. What about fxLayoutAlign?
  2. What about fxFlex="33%" and the other variations?
  3. How does CSS Grid play into this?
  4. Would the library have an API that doesn't map directly to Grid/Flexbox and be able to determine when it was best to use Grid vs Flexbox?

@CaerusKaru
Copy link
Member Author

CaerusKaru commented May 26, 2020

@Splaktar <bp name="gt-xs" layout="row" gap="25px"> is totally valid, and is the intention of the design (consolidating by breakpoint).

  • (1 and 2) What's special about fxLayoutAlign and fxFlex="33%"? They should translate to align and flex respectively. (fxFlexAlign translates to flex-align, but this is not dogmatic)
  • CSS Grid is the exact same interface (e.g. alignColumns, gridGap, etc)
  • No but no reason that can't be a follow-up feature

@Splaktar
Copy link
Member

Thank you for the quick response! I'm just starting to dig into the full README.md

Copy link
Member

@Splaktar Splaktar left a 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!

Comment on lines +26 to +27
<bp tag="xs" flexAlign="end"></bp>
<bp tag="md" flexAlign="center"></bp>
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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]`})
Copy link
Member

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 {
Copy link
Member

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;
Copy link
Member

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', {
Copy link
Member

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 {
Copy link
Member

Choose a reason for hiding this comment

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

In this implementation, Tags are attributes? I think that this can be easily confused with HTML tags.

@samal-rasmussen
Copy link

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

@CaerusKaru
Copy link
Member Author

@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.

@epelc
Copy link
Contributor

epelc commented Jul 23, 2021

@CaerusKaru are there any plans for an automatic upgrade using ng update or similar? The feature set seems largely the same and like this could be supported.

Also is there a link to the discussion thread? I could find anything here.

@CaerusKaru
Copy link
Member Author

@epelc I would love to have some ng update functionality, and if we ever invested in this actively, that would definitely be a fast follow to release (again, if it were up to me). Feel free to add discussion directly to this PR. It would be too tedious to go back and forth on certain points between here and a tracking issue.

@ckorherr
Copy link

@Splaktar <bp name="gt-xs" layout="row" gap="25px"> is totally valid, and is the intention of the design (consolidating by breakpoint).

  • (1 and 2) What's special about fxLayoutAlign and fxFlex="33%"? They should translate to align and flex respectively. (fxFlexAlign translates to flex-align, but this is not dogmatic)
  • CSS Grid is the exact same interface (e.g. alignColumns, gridGap, etc)
  • No but no reason that can't be a follow-up feature

One thing is special about the align attribute, it is really badly rendered in vscode, see: microsoft/vscode#66723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes discussion Further discussion with the team is needed before proceeding rfc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants