Skip to content

/ angular

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

Jump to bottom

docs-infra: theming and dark mode #41129

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

docs-infra: theming and dark mode #41129

wants to merge 7 commits into from
+2,441 −950

Conversation

@AleksanderBodurri
Copy link
Contributor

@AleksanderBodurri AleksanderBodurri commented Mar 9, 2021 •
edited

Meant to continue the conversation from #40257.

This draft PR is a major refactor of the styles in https://angular.io/ in an effort to make aio themeable (primarily for a long awaited dark mode) and move towards using the sass module system.

The implementation is heavily inspired by the awesome folks who've worked on material.angular.io.

Previously styles were organized like this:

  • styles
    • 1-layout
      • _global_layout.scss
      • ...
    • 2-modules
      • _alert.scss
      • ...

This PR changes this structure to:

  • styles
    • 1-layout
      • global_layout
        • _global_layout.scss
        • _global_layout-theme.scss
      • ...
    • 2-modules
      • alert
        • _alert.scss
        • _alert-theme.scss
      • ...

Essentially splitting styling related to theming into separate files. Within each theme file is a scss mixin that takes in a material theme configuration and generates the appropriate styling for that theme and component.

Theme configurations are defined in: src/styles/custom-themes

Tooling that has been ported over from material.angular.io allows theme chunks to be lazy loaded.

The advantage of this approach:

  • Theming is contained to one file per component. Developers need only be aware of the existence of a single file when making theming related changes to a component
  • Moves away from using sass @imports that are no longer recommended by the sass team
  • Themes can be lazy loaded at runtime, preventing growth of the default styles chunk every time a new theme is implemented

disadvantages:

  • Splitting styles into theming files means duplication of some selectors, resulting in an increase of the default styles chunk size (in my testing this increase was about 2.6 KB on a prod build)

I'm opening this as a draft to request further technical review, and to start a conversation around what we want a potential dark mode to look like. This draft provides the tooling to make aio themeable, but does not provide an actual dark mode implementation. I didn't want to attempt an implementation without first getting direction from the design team.

EDIT: pushed a prototype dark mode implementation to iterate from.

EDIT: PR is ready to review 🔍

PR Type

What kind of change does this PR introduce?

  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

  • Yes
  • No
@google-cla google-cla bot added the cla: yes label Mar 9, 2021
@AleksanderBodurri AleksanderBodurri mentioned this pull request Mar 9, 2021
Closed
4 of 14 tasks complete
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch 3 times, most recently from e42dbd6 to 8cd2a5c Mar 9, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Mar 10, 2021

You can preview dd399cf at https://pr41129-dd399cf.ngbuilds.io/.
You can preview e42dbd6 at https://pr41129-e42dbd6.ngbuilds.io/.
You can preview 8cd2a5c at https://pr41129-8cd2a5c.ngbuilds.io/.
@ngbot ngbot bot added this to the Backlog milestone Mar 11, 2021
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 3ceb5ef to 77ab271 Mar 12, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Mar 12, 2021

You can preview 77ab271 at https://pr41129-77ab271.ngbuilds.io/.
@AleksanderBodurri
Copy link
Contributor Author

@AleksanderBodurri AleksanderBodurri commented Mar 12, 2021 •
edited

Implemented a prototype dark mode to iterate from.

Unrelated but is the ";;" at the bottom of the page on https://angular.io/ supposed to be there? Noticed it while working on this.

Screen Shot 2021-03-12 at 3 08 49 PM
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 77ab271 to cee8e1f Mar 12, 2021
@gkalpak
Copy link
Member

@gkalpak gkalpak commented Mar 12, 2021

The ;; at the end are definitely not supposed to be there 😁
I accidentally introduced them in #41162 😇 They will be fixed by #41183.
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Mar 12, 2021

You can preview cee8e1f at https://pr41129-cee8e1f.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch 2 times, most recently from b8a3a7d to 4931453 Mar 13, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Mar 13, 2021

You can preview 4931453 at https://pr41129-4931453.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 4931453 to f35ca19 Mar 13, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Mar 13, 2021

You can preview f35ca19 at https://pr41129-f35ca19.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri changed the title [RFC] aio Theming aio Theming Mar 13, 2021
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from f35ca19 to 85b7010 Mar 13, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Mar 13, 2021

You can preview 85b7010 at https://pr41129-85b7010.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 85b7010 to 93c32fc Mar 13, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Mar 13, 2021

You can preview 93c32fc at https://pr41129-93c32fc.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 93c32fc to 083086a Mar 13, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Mar 13, 2021

You can preview 083086a at https://pr41129-083086a.ngbuilds.io/.
@AleksanderBodurri
Copy link
Contributor Author

@AleksanderBodurri AleksanderBodurri commented Mar 13, 2021 •
edited

Encountered a weird bug while working on this. When I install aio with the install prompt in the url bar, the installed aio opens as expected but when I click the toggle for dark mode all the content on the screen becomes invisible. On mac os chrome 89.0.4389.90. Confirmed that this does not happen on windows.

  1. Open https://pr41129-93c32fc.ngbuilds.io/
  2. Install with the "install app" prompt in the url bar
  3. Click the dark mode toggle.

Can anyone confirm being able to reproduce this on their end?
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 083086a to 7927057 Mar 13, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Mar 13, 2021

You can preview 7927057 at https://pr41129-7927057.ngbuilds.io/.
@jelbourn jelbourn removed request for IgorMinar and atscott May 18, 2021
@jelbourn
jelbourn approved these changes May 18, 2021
View changes
@jelbourn
Copy link
Member

@jelbourn jelbourn commented May 18, 2021

@gkalpak any final comments on this?
@gkalpak
gkalpak approved these changes May 19, 2021
View changes
Copy link
Member

@gkalpak gkalpak left a comment

Amazing work, @AleksanderBodurri (and everyone chiming in/reviewing) 🤯 💯 🥇

I have left a few comments, but I would be fine with dealing with (most of) them in a follow-up PR (since this has already gotten quite big).

Also, I noticed a couple of issues on smaller screens (due to increasing the items we try to cram in the limited horizontal space of the toolbar.

At around 480px width, the search-bar overlaps with the Angular logo:

searchbar-issue-on-small

On mobile phones (e.g. at 360px width), some of the social icons overflow out of the screen (with no indication they exist or a way to get to them). Maybe we could consider moving them into the sidenav on smaller screens?

homescreen-on-mobile
aio/src/app/app.component.html Outdated Show resolved Hide resolved
aio/src/styles/1-layouts/marketing-layout/_marketing-layout-theme.scss Outdated Show resolved Hide resolved
aio/src/styles/0-base/_typography-theme.scss Outdated Show resolved Hide resolved
aio/src/app/app.module.ts Outdated Show resolved Hide resolved
aio/src/styles/2-modules/toc/_toc.scss Show resolved Hide resolved
aio/src/app/shared/theme-picker/theme-toggle.component.ts Outdated Show resolved Hide resolved
aio/src/app/shared/theme-picker/theme-toggle.component.ts Outdated Show resolved Hide resolved
aio/src/app/shared/theme-picker/theme-toggle.component.ts Outdated Show resolved Hide resolved
aio/src/styles/1-layouts/marketing-layout/_marketing-layout-theme.scss Outdated Show resolved Hide resolved
aio/src/app/app.component.html Outdated Show resolved Hide resolved
gkalpak added a commit to gkalpak/angular that referenced this pull request May 23, 2021
@gkalpak
refactor(docs-infra): use a service to access local-/sessionStorage
e8ce728 
Previously, we had the same logic in a couple of places to safely access
the `Window`'s `local-/sessionStorage` and provide a no-op fallback if
necessary. In the near future, we will need to same logic in more places
(for example, the light/dark theme toggle in angular#41129 and the cookies
popup for angular#42209).

This commit reduces code duplication by encapsulating the logic in a
`StorageService`. This also makes it easier to mock the storage in tests
without having to mess with the actual `Window` object.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch 3 times, most recently from 66195fa to 9d79153 May 25, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented May 25, 2021

You can preview 9d79153 at https://pr41129-9d79153.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 9d79153 to 93bf826 May 25, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented May 25, 2021

You can preview 93bf826 at https://pr41129-93bf826.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 93bf826 to 9a73356 May 25, 2021
@AleksanderBodurri
Copy link
Contributor Author

@AleksanderBodurri AleksanderBodurri commented May 25, 2021 •
edited

@gkalpak I think I've addressed most of your comments. There was one I was uncertain of so I left it open for clarification.

Appreciate the review 🙏 Let me know if theres anything else I can do for this PR.
@mary-poppins
Copy link

@mary-poppins mary-poppins commented May 25, 2021

You can preview 9a73356 at https://pr41129-9a73356.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 9a73356 to c800bca May 25, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented May 25, 2021

You can preview c800bca at https://pr41129-c800bca.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from c800bca to c918995 May 25, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented May 25, 2021

You can preview c918995 at https://pr41129-c918995.ngbuilds.io/.
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from c918995 to 4339e5c May 27, 2021
@AleksanderBodurri
feat(docs-infra): port over material io theming logic to angular io
57fc798 
brings in theming tools from material io into angular io in preparation of implementing darkmode
@AleksanderBodurri
refactor(docs-infra): rename namespaced scss to index files
1b8af62 
scss files were forwarded from files that were named without convention, changes these file names to follow conventions
@AleksanderBodurri
refactor(docs-infra): grab scss constants using scss use syntax inste…
38c2503 
…ad of the global imports

move away from using global constants in scss files because @import will be deprecated soon
@AleksanderBodurri
refactor(docs-infra): grab scss mixins with scss use syntax instead o…
25cc1be 
…f global imports

move away from global mixins because @import is going to be deprecated
@AleksanderBodurri
feat(docs-infra): create typography and layout theme files
eeb653e 
creates theming files for the aio typography styles; done as part of the effort to make aio themeable
@AleksanderBodurri
feat(docs-infra): create module theme files
efbdd63 
creates theming files for the module styles in aio; done as part of the effor to make aio themeable
@AleksanderBodurri
feat(docs-infra): implement dark mode
7ca90ae 
defines styles for a first iteration of an aio darkmode
@AleksanderBodurri AleksanderBodurri force-pushed the AleksanderBodurri:aio-theming branch from 4339e5c to 7ca90ae May 27, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented May 27, 2021

You can preview 7ca90ae at https://pr41129-7ca90ae.ngbuilds.io/.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alan-agius4 alan-agius4

@petebacondarwin petebacondarwin

@jelbourn jelbourn

@gkalpak gkalpak

@twerske twerske

@aikidave aikidave

@jessicajaniuk jessicajaniuk

Assignees
No one assigned
Labels
action: cleanup aio: preview cla: yes comp: docs-infra target: patch
Projects
None yet
Milestone
Backlog
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
@AleksanderBodurri @mary-poppins @gkalpak @jelbourn @twerske @petebacondarwin @crisbeto @alan-agius4 @aikidave @jessicajaniuk