Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings
This repository was archived by the owner on Jan 6, 2025. It is now read-only.

rfc: new layout system #1185

Open
CaerusKaru wants to merge 1 commit into master
base: master
Choose a base branch
Loading
from adam/one
Open

rfc: new layout system #1185

CaerusKaru wants to merge 1 commit into master from adam/one

Conversation

@CaerusKaru
Copy link
Member

@CaerusKaru CaerusKaru commented Feb 2, 2020 •
edited
Loading

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.

vmasek, LoganDupont, Splaktar, fxck, puku0x, charsleysa, JorisDebonnet, ccjmne, heddendorp, abdatta, and 11 more reacted with thumbs up emoji anisabboud reacted with thumbs down emoji JorisDebonnet, abdatta, ckorherr, and pabloiocampo reacted with heart emoji ccjmne, abdatta, jgbpercy, nzbin, and pabloiocampo reacted with eyes emoji
@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
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
Copy link

Is there any estimated release date for this ?
vdumbrav, erkanmaras, dymo, and pabloiocampo reacted with thumbs up emoji

Copy link
Contributor

Is there any estimated release date for this ?

I don't believe so.

Copy link
Contributor

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?

Copy link
Member Author

CaerusKaru commented May 26, 2020 •
edited
Loading

@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 and pabloiocampo reacted with thumbs up emoji

Copy link
Contributor

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

Copy link
Contributor

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

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
Contributor

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?

JorisDebonnet reacted with thumbs up emoji

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
Contributor

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>

kbarendrecht, JorisDebonnet, VIKTORVAV99, abdatta, nevech, ybeauchamph, adilbenmoussa, the-ult, EdricChan03, stagefright5, and 3 more reacted with thumbs up emoji
* tagging an HTML element as part of the layout system, and then
* coordinating all updates with GrandCentral.
*/
@Directive({selector: `[ngl]`})
Copy link
Contributor

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
Contributor

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?

surendra-y and PowerKiKi reacted with thumbs up emoji
/** 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
Contributor

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.

PowerKiKi reacted with thumbs up emoji
* 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
Contributor

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.

mackelito and PowerKiKi reacted with thumbs up emoji
* associated name, builder, cache, and dependencies on other builder input
* values.
*/
export abstract class Tag {
Copy link
Contributor

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.

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

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.
fxck and samal-rasmussen reacted with thumbs up emoji

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.

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.

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Splaktar Splaktar Splaktar left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes discussion Further discussion with the team is needed before proceeding rfc

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /