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-experimental/mdc-card): remove extra margin if header doesn't have an avatar #19072

Merged
merged 1 commit into from
Mar 3, 2022
Merged

fix(material-experimental/mdc-card): remove extra margin if header doesn't have an avatar #19072

merged 1 commit into from
Mar 3, 2022

Conversation

crisbeto
Copy link
Member

The card header margin was set up assuming that there would always be an avatar element, but that's not always the case. These changes add an extra check to avoid the extra margin.

Fixes #19069.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Apr 14, 2020
@crisbeto crisbeto requested a review from jelbourn as a code owner April 14, 2020 15:02
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 14, 2020
@jelbourn
Copy link
Member

Does the mdc-based card do something similar?

@crisbeto
Copy link
Member Author

Looks like it does, but we'd have to handle it differently since it uses padding to do the spacing. Do you remember the context behind it @jelbourn (you're on the blame)? https://github.com/angular/components/blob/master/src/material-experimental/mdc-card/card.scss#L33

@jelbourn
Copy link
Member

IIRC, this was the best I could do to preserve the existing spacing for the content regions without adding spacing to the card itself. Padding contained the spacing to only the specific content regions, which made the styles much simpler; when it was margin, the styles needed to be more aware of adjacent siblings and the like.

@crisbeto crisbeto force-pushed the 19069/calendar-header-text-margin branch from 533e9a6 to 0a447e8 Compare April 17, 2020 05:02
@crisbeto crisbeto requested a review from mmalerba as a code owner April 17, 2020 05:02
@crisbeto
Copy link
Member Author

I've pushed a fix to cover the MDC card @jelbourn.

@mmalerba
Copy link
Contributor

Can you add a demo app example (one for each version) that demonstrates this situation?

@crisbeto
Copy link
Member Author

Done.

@crisbeto crisbeto force-pushed the 19069/calendar-header-text-margin branch from 0a447e8 to 4e889e6 Compare April 17, 2020 18:42
mmalerba
mmalerba approved these changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM

@mmalerba mmalerba added lgtm action: merge The PR is ready for merge by the caretaker labels Apr 17, 2020
@jelbourn jelbourn added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 20, 2020
@jelbourn
Copy link
Member

This breaks about 61 targets worth of screenshot tests. All of them are cases where it looks like they were depending on this padding, so it breaks the app layout.

@saithis
Copy link

saithis commented May 25, 2020

@jelbourn Whats the plan for this fix then, will this be considered for v10?

@jelbourn
Copy link
Member

Due to how breaking this is, we probably won't land it at all.

@saithis
Copy link

saithis commented May 26, 2020 •
edited
Loading

This makes mat-card pretty annoying for us. We have to work around this on every usage with a title, as we never use the avatar element :/

@Totati
Copy link
Contributor

Totati commented May 26, 2020

This makes mat-card pretty annoying for us. We have to work around this on every usage with a title, as we never use the avatar element :/

You could fix it in your styles.scss file globally till mdc-card arrives.

@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@crisbeto crisbeto force-pushed the 19069/calendar-header-text-margin branch from 4e889e6 to 27f25e5 Compare April 2, 2021 09:59
@crisbeto crisbeto force-pushed the 19069/calendar-header-text-margin branch from 27f25e5 to b8309d6 Compare May 4, 2021 06:01
@crisbeto crisbeto changed the title fix(card): remove extra margin if header doesn't have an avatar fix(material/card): remove extra margin if header doesn't have an avatar May 4, 2021
@crisbeto
fix(material-experimental/mdc-card): remove extra margin if header do…
a360ff3 
…esn't have an avatar

The card header margin was set up assuming that there would always be an avatar element, but that's not always the case. These changes add an extra check to avoid the extra margin.

Fixes angular#19069.
@crisbeto crisbeto force-pushed the 19069/calendar-header-text-margin branch from b8309d6 to a360ff3 Compare May 4, 2021 17:17
@crisbeto crisbeto changed the title fix(material/card): remove extra margin if header doesn't have an avatar fix(material-experimental/mdc-card): remove extra margin if header doesn't have an avatar May 4, 2021
@crisbeto crisbeto removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label May 4, 2021
@crisbeto
Copy link
Member Author

crisbeto commented May 4, 2021

I've reduced the scope of these changes to only apply to the MDC card.

@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@zarend zarend added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Feb 4, 2022
@zarend
Copy link
Contributor

zarend commented Feb 4, 2022

Ran presubmit last night, looks like this has about 41 presubmit failures.

@andrewseguin andrewseguin merged commit 557f8bc into angular:master Mar 3, 2022
andrewseguin pushed a commit that referenced this pull request Mar 3, 2022
@crisbeto @andrewseguin
fix(material-experimental/mdc-card): remove extra margin if header do…
8414646 
…esn't have an avatar (#19072)

The card header margin was set up assuming that there would always be an avatar element, but that's not always the case. These changes add an extra check to avoid the extra margin.

Fixes #19069.

(cherry picked from commit 557f8bc)
@sync-by-unito sync-by-unito bot mentioned this pull request Mar 14, 2022
Closed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 18, 2022
Update angularmaterial monorepo to v13.2.6 (#1214)
fc919a2 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/cdk](https://github.com/angular/components) | dependencies | patch | [`13.2.5` -> `13.2.6`](https://renovatebot.com/diffs/npm/@angular%2fcdk/13.2.5/13.2.6) |
| [@angular/material](https://github.com/angular/components) | dependencies | patch | [`13.2.5` -> `13.2.6`](https://renovatebot.com/diffs/npm/@angular%2fmaterial/13.2.5/13.2.6) |

---

### Release Notes

<details>
<summary>angular/components</summary>

### [`v13.2.6`](https://github.com/angular/components/blob/HEAD/CHANGELOG.md#&#8203;1326-suede-spaghetti-2022-03-09)

[Compare Source](angular/components@13.2.5...13.2.6)

##### cdk

| Commit | Type | Description |
| -- | -- | -- |
| [39929a815d](angular/components@39929a8) | fix | **overlay:** backdrop timeouts not being cleared in some cases ([#&#8203;23972](angular/components#23972)) |
| [2f2b0c7cf4](angular/components@2f2b0c7) | fix | **testing:** dispatch mouseover and mouseout events in UnitTestElement ([#&#8203;24490](angular/components#24490)) |
| [edca54f2d0](angular/components@edca54f) | fix | **testing:** require at least one argument for locator functions ([#&#8203;23619](angular/components#23619)) |

##### material

| Commit | Type | Description |
| -- | -- | -- |
| [c4993ac171](angular/components@c4993ac) | fix | **button:** avoid setting a tabindex on all link buttons ([#&#8203;22901](angular/components#22901)) |
| [c47d30e0e5](angular/components@c47d30e) | fix | **dialog:** don't wait for animation before moving focus ([#&#8203;24121](angular/components#24121)) |
| [70b8248568](angular/components@70b8248) | fix | **expansion:** able to tab into descendants with visibility while closed ([#&#8203;24045](angular/components#24045)) |
| [d22d73ab8d](angular/components@d22d73a) | fix | **select:** disabled state out of sync when swapping form group with a disabled one ([#&#8203;17872](angular/components#17872)) |
| [911d6b71d4](angular/components@911d6b7) | fix | **slide-toggle:** clear name from host node ([#&#8203;15505](angular/components#15505)) |
| [4b5363d160](angular/components@4b5363d) | fix | **tooltip:** decouple removal logic from change detection ([#&#8203;19432](angular/components#19432)) |

##### material-experimental

| Commit | Type | Description |
| -- | -- | -- |
| [8414646d79](angular/components@8414646) | fix | **mdc-card:** remove extra margin if header doesn't have an avatar ([#&#8203;19072](angular/components#19072)) |
| [f66486dc5b](angular/components@f66486d) | fix | **mdc-slider:** fix a few null pointer exceptions ([#&#8203;23659](angular/components#23659)) |

##### multiple

| Commit | Type | Description |
| -- | -- | -- |
| [6ee0089ce6](angular/components@6ee0089) | fix | don't block child component animations on open ([#&#8203;24529](angular/components#24529)) |

#### Special Thanks

Andrew Seguin, Jeri Peier, Kristiyan Kostadinov and Paul Gschwendtner

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1214
Reviewed-by: Epsilon_02 <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
@sync-by-unito sync-by-unito bot mentioned this pull request Mar 22, 2022
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mmalerba mmalerba mmalerba approved these changes

@jelbourn jelbourn Awaiting requested review from jelbourn

Assignees

@mmalerba mmalerba

Labels
action: merge The PR is ready for merge by the caretaker P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged 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.

mat-card-(sub)title wrong margin
8 participants
@crisbeto @jelbourn @mmalerba @saithis @Totati @zarend @googlebot @andrewseguin