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.

Sign up for GitHub

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

fix(material/list): nav list item border-radius #28789

Merged
merged 3 commits into from Apr 10, 2024
Merged

fix(material/list): nav list item border-radius #28789

merged 3 commits into from Apr 10, 2024

Conversation

wagnermaciel
Copy link
Contributor

@wagnermaciel wagnermaciel requested review from crisbeto and removed request for devversion March 27, 2024 20:24
@crisbeto crisbeto added the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Mar 27, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 27, 2024 •
edited

Deployed dev-app for e6e6614 to: https://ng-dev-previews-comp--pr-angular-components-28789-dev-brlh8s9b.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

crisbeto
crisbeto approved these changes Mar 27, 2024
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

The list change LGTM. I wonder if we don't need to make a change to either the inner padding of the list or the sidenav since it currently looks a bit weird because it's edge-to-edge 🤔

@wagnermaciel
Copy link
Contributor Author

wagnermaciel commented Mar 27, 2024 •
edited

The list change LGTM. I wonder if we don't need to make a change to either the inner padding of the list or the sidenav since it currently looks a bit weird because it's edge-to-edge 🤔

I was wondering this too. I think we do want this, but that change is probably so breaking that I'll want to do it in a separate PR 😭

Also, I never knew about that dev-app-preview label. That is awesome!

@crisbeto
Copy link
Member

I think that if we scope it to only nav lists inside sidenavs on M3 it shouldn't be too breaking.

@wagnermaciel wagnermaciel added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Apr 10, 2024
@wagnermaciel wagnermaciel removed the request for review from mmalerba April 10, 2024 20:37
@wagnermaciel wagnermaciel merged commit 7165a45 into angular:main Apr 10, 2024
26 of 28 checks passed
wagnermaciel added a commit that referenced this pull request Apr 10, 2024
@wagnermaciel
fix(material/list): nav list item border-radius (#28789)
36a1d45 
* fix(material/list): nav list item border-radius

* https://m2.material.io/components/navigation-drawer#states
* https://m3.material.io/components/navigation-drawer/specs

* fixup! fix(material/list): nav list item border-radius

* fixup! fix(material/list): nav list item border-radius

(cherry picked from commit 7165a45)
@karmasakshi
Copy link

I'm on M2 and this change has introduced border-radius to menu items. Is this expected? Screenshot from my local:

Screen Shot 2024-04-12 at 7 51 30 PM

@wagnermaciel
Copy link
Contributor Author

@karmasakshi Hi Karma, yes this is intentional. Here's what the m2 nav list (or navigation drawer as Material Design calls it) is meant to look like

https://m2.material.io/components/navigation-drawer

@karmasakshi
Copy link

karmasakshi commented Apr 12, 2024 •
edited

Thank you, I added an overall padding to .mdc-list instead of vertical only. Looks good.

Screen Shot 2024-04-12 at 8 01 51 PM Screen Shot 2024-04-12 at 8 14 41 PM

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

@crisbeto crisbeto crisbeto approved these changes

@zarend zarend zarend approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker dev-app preview When applied, previews of the dev-app are deployed to Firebase target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wagnermaciel @crisbeto @karmasakshi @zarend