Skip to content

docs: explicitly set form field appearance #22837

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

Merged

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented May 28, 2021

Currently the default form field appearance is legacy, but we switch it to fill only in Stackblitz since that's what we want to change the default to eventually. The problem with this approach is that the default is only changed on Stackblitz, not the docs site that user has to go through.

These changes set the appearance to fill explicitly so that it is consistent.

Companion PR for the docs site: angular/material.angular.io#1002

Currently the default form field appearance is `legacy`, but we switch it to `fill` only in Stackblitz since that's what we want to change the default to eventually. The problem with this approach is that the default is only changed on Stackblitz, not the docs site that user has to go through.

These changes set the appearance to `fill` explicitly so that it is consistent.
@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent docs This issue is related to documentation merge safe labels May 28, 2021
@crisbeto crisbeto requested a review from mmalerba May 28, 2021 17:00
@crisbeto crisbeto requested a review from andrewseguin as a code owner May 28, 2021 17:00
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 28, 2021
crisbeto added a commit to crisbeto/material.angular.io that referenced this pull request May 28, 2021
Companion PR to angular/components#22837. Removes the default form field appearance so that we get a consistent result between the docs site and Stackblitz.
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 action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels May 28, 2021
@mmalerba
Copy link
Contributor

This has conflicts with the 12.0.x branch, needs another PR for that branch so the patch release docs don't lose their fill appearance when we merge the m.aio PR

@crisbeto
Copy link
Member Author

crisbeto commented May 29, 2021

I think that it won't be a big deal if we make the docs site change now and this waits until the next minor release. The current behavior is already broken since the changed default only applies when forked to Stackblitz, not on the docs site itself.

@crisbeto crisbeto added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels May 29, 2021
@mmalerba
Copy link
Contributor

mmalerba commented Jun 1, 2021

The thing is we don't have separate release branches on angular/material.angular.io so the docs site change will wind up applying to both patch and minor releases. Therefore we need this to land on both branches

@crisbeto
Copy link
Member Author

crisbeto commented Jun 1, 2021

I don't think that's a big deal either. I think that the current behavior is more broken compared to the behavior after this change.

@mmalerba
Copy link
Contributor

mmalerba commented Jun 1, 2021

I'll at least merge this for now. If we don't want the appearance to change on the form-fields we can hold off on merging the docs site change until 12.1.0 comes out

@mmalerba mmalerba merged commit 21b81d6 into angular:master Jun 1, 2021
@Splaktar
Copy link
Contributor

Shouldn't we be changing the component's default to fill again instead of doing this? Do we plan on doing that for v12.1 or v13? Is there an associated issue for that?

crisbeto added a commit to angular/material.angular.io that referenced this pull request Jun 23, 2021
Companion PR to angular/components#22837. Removes the default form field appearance so that we get a consistent result between the docs site and Stackblitz.
@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 Jul 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement docs This issue is related to documentation P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants