Skip to content

docs(sidenav): update responsive example to use current APIs - #33318

Open
EduardF1 wants to merge 3 commits into
angular:mainfrom
EduardF1:docs/update-responsive-sidenav-example
Open

EduardF1 wants to merge 3 commits into
angular:mainfrom
EduardF1:docs/update-responsive-sidenav-example

Conversation

@EduardF1

@EduardF1 EduardF1 commented May 29, 2026 •

Copy link
Copy Markdown

Summary

Updates the responsive sidenav documentation example to use the current CDK APIs instead of manually managing MediaMatcher/MediaQueryList event listeners.

Root Cause

The example was using MediaMatcher with addEventListener/removeEventListener, which requires manual lifecycle management via OnDestroy. An Angular team member noted the preferred approach is to use the CDK's BreakpointObserver.observe() directly.

Changes

  • Replaced MediaMatcher with BreakpointObserver.observe()
  • Used takeUntilDestroyed() for automatic subscription cleanup
  • Removed OnDestroy implementation and manual event listener management
  • Removed private _mobileQuery and _mobileQueryListener fields

Result

The component is now simpler, idiomatic Angular, and no longer uses the raw MediaQueryList API.

Fixes #29266

@google-cla

google-cla Bot commented May 29, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@angular-robot angular-robot Bot added the area: docs Related to the documentation label May 29, 2026
@pullapprove
pullapprove Bot requested review from crisbeto and wagnermaciel May 29, 2026 23:14
@EduardF1
EduardF1 force-pushed the docs/update-responsive-sidenav-example branch from fc2eb8f to ff0fc7d Compare May 31, 2026 11:02
@EduardF1

Copy link
Copy Markdown
Author

Update: I have already signed the Google CLA with fischerszavarduard@gmail.com, and I have now rewritten this PR commit so the author/committer email matches that signed address. The cla/google check is still failing. Could a maintainer please re-run or investigate?

@EduardF1
EduardF1 force-pushed the docs/update-responsive-sidenav-example branch from ff0fc7d to bde711d Compare June 4, 2026 00:12
@EduardF1

EduardF1 commented Jun 4, 2026

Copy link
Copy Markdown
Author

Just following up on this PR. The checks look good (apart from the CLA which I believe needs separate handling). Let me know if I need to make any changes!

@EduardF1

EduardF1 commented Jun 4, 2026

Copy link
Copy Markdown
Author

Hi! Just checking in on this PR. The CI is green. Is there anything else needed from my side to help get this reviewed and merged?

@EduardF1
EduardF1 force-pushed the docs/update-responsive-sidenav-example branch from bde711d to 4c9546e Compare June 10, 2026 07:25
@EduardF1

Copy link
Copy Markdown
Author

@googlebot I signed it!

@EduardF1
EduardF1 force-pushed the docs/update-responsive-sidenav-example branch from 4c9546e to 8342202 Compare June 30, 2026 12:46
Replace the manual MediaMatcher/MediaQueryList setup with the CDK's
BreakpointObserver.observe(), which is the idiomatic Angular approach.

Use takeUntilDestroyed() for automatic subscription cleanup, removing
the need to implement OnDestroy and manually manage event listeners.

Fixes angular#29266
@pullapprove
pullapprove Bot requested a review from tjshiu June 30, 2026 12:46
this._mobileQueryListener = () => this.isMobile.set(this._mobileQuery.matches);
this._mobileQuery.addEventListener('change', this._mobileQueryListener);
}
const breakpointObserver = inject(BreakpointObserver);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good catch to use the breakpoint observer. A lot cleaner.

IMO I would take this one step further: pull this up into making isMobile a toSignal of this breakpoint stream.

I made sure this worked before I suggested it, so if you are interested:

  protected readonly isMobile = toSignal(
    inject(BreakpointObserver)
      .observe('(max-width: 600px)')
      .pipe(map((result) => result.matches)),
    { requireSync: true }
  );

One other thing, unrelated to your changes themself, but this example file could use: tracking errors are thrown in the console due to the current tracking of the items in the string arrays can have duplicates. With no unique property to drill into, I think tracking by $index would be optimal.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the review, and for checking that the toSignal version works. I agree it is cleaner: it drops the manual subscription entirely and reads better in the template. I have pushed that change (isMobile as a toSignal of the BreakpointObserver stream with requireSync: true).

On the tracking of the string arrays: good catch, the duplicate values do produce tracking warnings. Since it is unrelated to this change I have left it out to keep the diff focused, but it is a one-line switch to $index in the same file, so if a maintainer would rather see it folded into this PR I am happy to add it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: docs Related to the documentation area: material/sidenav

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs-bug(Sidenav): deprecated methods used in the responsive sidenav example

3 participants