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.

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

fix(form-field): remove nonbreaking space before * for required fields #15490

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@stevenyxu
Copy link
Contributor

stevenyxu commented Mar 14, 2019

The surrounding .mat-form-field-label is already white-space: nowrap so the   doesn't serve any
wrap-preventing function. But it does prevent any trailing spacing from collapsing with
the space before the asterisk, causing for a bad visual appearance of a double space in these cases.

Addresses #15489.

I attempted to use a literal space and not the symbol, but the compiler collapsed it for me.

@stevenyxu stevenyxu requested a review from mmalerba as a code owner Mar 14, 2019

@googlebot googlebot added the cla: yes label Mar 14, 2019

@stevenyxu stevenyxu force-pushed the stevenyxu:form-field branch from 77faefd to eda555b Mar 14, 2019

@stevenyxu

This comment has been minimized.

Copy link
Contributor Author

stevenyxu commented Mar 14, 2019

I tried to add a test to check in a more deliberate way that the spaces were deliberate and there's no real DOM API I found to get the post-collapsing rendered text short of doing a screendiff test. Both innerText and textContent give the spaces pre-collapsing. I did adjust the test to match explicitly literal space and not \s which also matches non-breaking space, so the test would have failed without the change.

I did not add an explicit test that puts a trailing space in the placeholder or uses a mat-label because I wasn't sure it was worth a test. I have a change ready for it so if you want me to add it, let me know.

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Mar 15, 2019

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "ci/circleci: bazel_build_test" is failing
    failure status "ci/circleci: tests_browserstack" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mmalerba

This comment has been minimized.

Copy link
Contributor

mmalerba commented Mar 15, 2019

@stevenyxu looks like there's some failing tests

@stevenyxu stevenyxu force-pushed the stevenyxu:form-field branch 2 times, most recently from 5e76466 to f5f25c6 Mar 15, 2019

fix(form-field): remove nonbreaking space before * for required fields
The surrounding .mat-form-field-label is already white-space: nowrap so the   doesn't serve any
wrap-preventing function. But it does prevent any trailing <mat-label> spacing from collapsing with
the space before the asterisk, causing for a bad visual appearance of a double space in these cases.

@stevenyxu stevenyxu force-pushed the stevenyxu:form-field branch from f5f25c6 to 3dcdb6d Mar 18, 2019

@stevenyxu

This comment has been minimized.

Copy link
Contributor Author

stevenyxu commented Mar 18, 2019

@mmalerba sorry about that. Fixed and rebased today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.