Stepper alternative label by IlCallo · Pull Request #12674 · angular/components

IlCallo · 2018-08-14T15:30:02Z

Added the "alternative label" version, as by (old) spec.

Tryed on Chrome, Firefox, Edge and IE11 and everything seems fine.
Added a paragraph in the stepper documentation and an example.

I'm not sure the code style is ok so if you have suggestions I'll be happy to clean it up a bit, just tell me how :)

googlebot · 2018-08-14T15:30:12Z

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

IlCallo · 2018-08-14T15:40:05Z

I already signed the CLA earlier today as a Corporate (Dreamonkey S.r.l.) of which I'm the Point of Contact.
I see that I've committed using the wrong email tho, but I'm not sure about how I can resolve the issue given the fact that I already created the PR.

I'm not even sure about how to fix the Can't find stylesheet to import @import './step-header'; error, the file is there and I succesfully built it locally.

IlCallo · 2018-08-21T14:50:18Z

I can confirm that the Corporate CLA has been accepted for my company

googlebot · 2018-08-21T14:50:24Z

CLAs look good, thanks!

googlebot · 2018-08-23T15:06:52Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

IlCallo · 2018-08-23T16:21:23Z

I squashed all commits in a single one and removed the import which gave problems, but it seems that now CLA is complaining.
I'm not really sure about which email I shall use for commits at this point: the one in the google group for the CLA is the same used for the squashed commit and it's my main email on Github, still the bot is complaining...

ngbot · 2018-08-23T20:33:42Z

Hi @IlCallo! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

mmalerba · 2018-08-23T21:23:38Z

It looks like you somehow wound up including a bunch of other commits from other authors, if it's too difficult to fix you can just move the changes to a new PR

googlebot · 2018-08-24T07:57:21Z

CLAs look good, thanks!

IlCallo · 2018-08-24T07:59:46Z

Everything should be fine now

mmalerba · 2018-08-24T16:50:51Z

 inputs: ['selectedIndex'],
 host: {
 'class': 'mat-stepper-horizontal',
+ '[class.mat-stepper--alternative-label]': 'alternativeLabel',


nit: I don't think we use the -- style anywhere else in our code, use - for consistency

mmalerba · 2018-08-26T00:52:27Z

+ margin: 0;
+ min-width: 0;
+ position: relative;
+ top: $mat-stepper-side-gap + $mat-stepper-label-header-height/2;


nit: put space around '/'

mmalerba · 2018-08-26T00:52:46Z

+ display: inline-block;
+ height: 0;
+ position: absolute;
+ top: $mat-stepper-side-gap + $mat-stepper-label-header-height/2;


nit: space around '/'

mmalerba · 2018-08-26T00:55:42Z

+ height: 0;
+ position: absolute;
+ top: $mat-stepper-side-gap + $mat-stepper-label-header-height/2;
+ width: calc(50% - #{$mat-stepper-label-header-height/2} - #{$mat-stepper-line-gap});


you can combine the second and third terms in SASS:
calc(50% - #{$mat-stepper-label-header-height/2-$mat-stepper-line-gap});

mmalerba · 2018-08-26T00:56:26Z

 margin-left: $mat-stepper-line-gap;
 }
 }
+ .mat-stepper-alternative-label & {


mmalerba · 2018-08-26T00:56:37Z

+ &:not(:last-child)::after {
+ @extend %mat-header-horizontal-line-alternative-label;
+ right: 0;
+ }


mmalerba · 2018-08-26T00:56:46Z

+ &:not(:first-child)::before {
+ @extend %mat-header-horizontal-line-alternative-label;
+ left: 0;
+ }


mmalerba · 2018-08-26T00:56:53Z

+ & .mat-step-icon,
+ & .mat-step-icon-not-touched {
+ margin-right: 0;
+ }


mmalerba · 2018-08-26T01:00:23Z

+export class MatHorizontalStepper extends MatStepper {
+ /** Whether the label should display in the alternative or normal mode. */
+ @Input()
+ get alternativeLabel(): boolean { return this._alternativeLabel; }


Although the spec calls it an "alternative label", I don't think it's an intuitive name to give it in the API. How about labelPosition which can have the following values: bottom - below the icon, end - after the icon (to the right in LTR / to the left in RTL). This would also leave open the possibility of future values like top and start

How should I change CSS classes name with the API you proposed?
label-bottom and label-end don't really seem good names...
I agree with you that alternativeLabel may not be the perfect name, but I'd prefer leaving it as an on/off property until someone actually needs to put it in top or start position...
(Sorry for delay, I was in vacation)

I'm not saying we should add support for start and top right now. I'm just saying that its more clear what the API does if we call it labelPosition and it leaves us open to adding new values in the future. I think class names like .mat-step-label-position-end would be good

I should have addressed all of yor concerns.
I did not changed previous code (I did not created a .mat-step-label-position-end SCSS class) to avoid breaking things (even if that class is actually added to the element for consistency).
Now the alternative label is applied when labelPosition is set to bottom

mmalerba · 2018-08-26T01:00:33Z

+ <button mat-button (click)="stepper.reset()">Reset</button>
+ </div>
+ </mat-step>
+</mat-horizontal-stepper>


mmalerba

It looks like you might need to add a few additional styles for RTL languages:

mmalerba · 2018-09-06T15:54:58Z

+ // We use auto instead of fixed 104px (by spec) because when there is an optional step
+ // the height is greater than that 
+ height: auto;
+ min-width: 100px;


where does this 100px come from?

Nowhere currently, the spec doesn't specify a minimum width (or I didn't find it) so I chose one.
Do you have any suggestions about?

Is having a min-width important to get it to lay out right? if not I would just leave it off and people can specify their own if they want. If we do need one just put it in an SCSS variable

It just shrinks until only the step icon is still visible, making the label useless.
Shall I remove it?

Yeah I think its best to just remove it and let people decide their own min

Fixed RTL and removed min-width

mmalerba · 2018-09-06T17:43:33Z

Sorry to be nit-picky, but it still looks slightly off in RTL:

vs LTR:

IlCallo · 2018-09-07T10:23:08Z

No problem, thank you for pointing it out, I didn't even see that :)
Should be fine now

mmalerba · 2018-09-07T16:29:25Z

Awesome, looks good aside from this lint error:

ERROR: /home/travis/build/angular/material2/src/lib/stepper/stepper.ts[11, 1]: All imports on this line are unused.

ngbot · 2018-09-18T21:12:36Z

Hi @IlCallo! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-09-18T21:13:11Z

Hi @IlCallo! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-09-18T21:13:34Z

Hi @IlCallo! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-09-18T21:14:28Z

Hi @IlCallo! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

Added the "alternative label" version, as by [(old) spec](https://material.io/archive/guidelines/components/steppers.html#steppers-types-of-steppers). Tryed on Chrome, Firefox, Edge and IE11 and everything seems fine. Added a paragraph in the stepper documentation and an example. I'm not sure the code style is ok so if you have suggestions I'll be happy to clean it up a bit, just tell me how :)

angular-automatic-lock-bot · 2019-09-09T22:01:41Z

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.}

IlCallo requested review from amcdnl, jelbourn and mmalerba as code owners August 14, 2018 15:30

googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Aug 14, 2018

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Aug 21, 2018

IlCallo requested review from andrewseguin, crisbeto and devversion as code owners August 23, 2018 15:06

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Aug 23, 2018

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Aug 24, 2018

mmalerba reviewed Aug 24, 2018
View reviewed changes

mmalerba reviewed Aug 26, 2018
View reviewed changes

mmalerba reviewed Sep 6, 2018
View reviewed changes

feat(stepper): add alternative label version
88e64d3

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Sep 10, 2018

Merge branch 'master' into stepper-alternative-label
f9e072e

mmalerba merged commit e4c6cc9 into angular:masterSep 21, 2018

IlCallo deleted the stepper-alternative-label branch October 15, 2018 14:54

IlCallo mentioned this pull request Oct 22, 2018
bug(stepper) labelPosition wrong GUI if 'dir' attribute is missing #13741
Closed

angular-automatic-lock-botBot locked and limited conversation to collaborators Sep 9, 2019

Uh oh!

Conversation

IlCallo commented Aug 14, 2018

Uh oh!

googlebot commented Aug 14, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

IlCallo commented Aug 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IlCallo commented Aug 21, 2018

Uh oh!

googlebot commented Aug 21, 2018

Uh oh!

googlebot commented Aug 23, 2018

Uh oh!

IlCallo commented Aug 23, 2018

Uh oh!

ngbotBot commented Aug 23, 2018

Uh oh!

mmalerba commented Aug 23, 2018

Uh oh!

googlebot commented Aug 24, 2018

Uh oh!

IlCallo commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmalerba commented Sep 6, 2018

Uh oh!

IlCallo commented Sep 7, 2018

Uh oh!

mmalerba commented Sep 7, 2018

Uh oh!

ngbotBot commented Sep 18, 2018

Uh oh!

ngbotBot commented Sep 18, 2018

Uh oh!

ngbotBot commented Sep 18, 2018

IlCallo commented Aug 14, 2018•
edited
Loading