Skip to content

Conversation

raygervais
Copy link
Contributor

Fixes #10381 by adding documentation examples for edit and option steps.
StackBlitz Demo: https://angular-material2-issue-gebvgu.stackblitz.io

@googlebot
Copy link

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
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 project maintainer to go/cla#troubleshoot.
  • 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.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Mar 23, 2018
@raygervais
Copy link
Contributor Author

raygervais commented Mar 23, 2018 via email

@googlebot
Copy link

CLAs look good, thanks!

@googlebot 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 Mar 23, 2018
@jelbourn jelbourn requested a review from mmalerba March 23, 2018 16:11
<button mat-raised-button (click)="isLinear = true" id="toggle-linear">Enable linear mode</button>

<button mat-raised-button (click)="isLinear = !isLinear" id="toggle-linear">
{{!isLinear?'Enable linear mode':'Disable linear mode' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spaces around ?, spaces around :, no space before: }}

@@ -1,5 +1,6 @@
<button mat-raised-button (click)="isLinear = true" id="toggle-linear">Enable linear mode</button>

<button mat-raised-button (click)="isLinear = !isLinear" id="toggle-linear">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why this had an ID in the first place, but we can remove it

@@ -0,0 +1,37 @@
<button mat-raised-button (click)="isEditable = !isEditable" id="toggle-linear">
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ID

@@ -0,0 +1,37 @@
<button mat-raised-button (click)="isEditable = !isEditable" id="toggle-linear">
{{!isEditable?'Enable edit mode':'Disable edit mode' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

same spacing comment as above

@@ -0,0 +1,37 @@
<button mat-raised-button (click)="isEditable = !isEditable" id="toggle-linear">
{{!isEditable?'Enable edit mode':'Disable edit mode' }}
</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

match indentation with open <button> tag

* @title Stepper type - editable
*/
@Component({
selector: 'stepper-overview-example',
Copy link
Contributor

Choose a reason for hiding this comment

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

Update selector, templateUrl, and styleUrls

@@ -0,0 +1,37 @@
<button mat-raised-button (click)="isOptional = !isOptional" id="toggle-linear">
Copy link
Contributor

Choose a reason for hiding this comment

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

same nit comments as previous files

import {FormBuilder, FormGroup, Validators} from '@angular/forms';

/**
* @title Stepper type - optional
Copy link
Contributor

Choose a reason for hiding this comment

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

@title Stepper with optional steps

firstCtrl: ['', Validators.required]
});
this.secondFormGroup = this._formBuilder.group({
secondCtrl: ['',]
Copy link
Contributor

Choose a reason for hiding this comment

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

can just do ''

templateUrl: 'stepper-type-optional-example.html',
styleUrls: ['stepper-type-optional-example.css']
})
export class StepperTypeOptionalExample {
Copy link
Contributor

Choose a reason for hiding this comment

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

stepper-optional/, stepper-optional-example.*, and StepperOptionalExample

@raygervais raygervais changed the title [WIP] docs(stepper): Improves documentation for stepper component docs(stepper): Improves documentation for stepper component Mar 24, 2018
@raygervais
Copy link
Contributor Author

I fixed the issues which you had found @mmalerba, and also thought of another two example which perhaps we'd want documented: Horizontal vs Vertical Steppers, Keyboard interactions. Thoughts? I'd be happy to add these to the current PR if you think they are useful in the documentation.

@mmalerba
Copy link
Contributor

@raygervais Yep those sound like good things to add. The current batch of examples looks good to me, so I can either approve this one and you do the other examples in a separate PR or you can just add them to this PR and I'll wait until you're done. Let me know, thanks for your work on this!

@raygervais
Copy link
Contributor Author

I'll do an add-on PR after this one is merged since I won't be able to do any work today. I can have it for you by tomorrow estimating. Glad to hear you're in agreement with the extra examples I thought of 😄

@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Mar 25, 2018
@mmalerba mmalerba added docs This issue is related to documentation target: patch This PR is targeted for the next patch release labels Mar 25, 2018
@mmalerba
Copy link
Contributor

@raygervais One thing I realized we forgot, can you replace the inline examples in the markdown file with comments that embed these? The overview example shows how to do this: https://github.com/angular/material2/blame/master/src/lib/stepper/stepper.md#L3

@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Mar 26, 2018
@raygervais
Copy link
Contributor Author

Doing so now @mmalerba :)

@@ -112,10 +112,52 @@ are completed.
If completion of a step in linear stepper is not required, then the `optional` attribute can be set
on `mat-step`.

```html
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you can remove these and put magic comments like for the overview example above:

<!-- example(stepper-overview) -->

Just replace stepper-overview with the name of the example folder and we have a script to embed it when we suck the markdown files into the docs site. (It comes out looking like this: https://material.angular.io/components/stepper/overview)

@raygervais
Copy link
Contributor Author

Corrected @mmalerba, glad to know there is magic syntax which makes importing example components a breeze! 👍

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Mar 29, 2018
@josephperrott
Copy link
Member

@raygervais can you rebase and push your PR again?

@raygervais raygervais force-pushed the stepper-documentation-update branch from 6d12402 to aad9b12 Compare March 30, 2018 21:35
@raygervais
Copy link
Contributor Author

Corrected @josephperrott, I hope the commit message style is as expected, I was unsure if footer was necessary in this case. Either or, happy to fix if need be 👍

@josephperrott
Copy link
Member

The commit message looks fine, thanks!

It looks like however there are lint and e2e failures, can you take look at those?

@raygervais raygervais force-pushed the stepper-documentation-update branch 2 times, most recently from 58cb56f to b0c7a2a Compare April 2, 2018 17:21
@raygervais
Copy link
Contributor Author

Tackled the lint issues, will tackle the E2E tests today 💯

Glad to know commit message is appropriate, was a major concern based on the expected format in the CONTRIBUTORS.md file.

Updates button logic setting button from true to !isLinear, including text indicator
of current state.
Creates editable and optional documentation examples
Imports into .MD file using special syntax

docs(stepper): Created stepper edit example
docs(stepper): Created stepper optional example
docs(stepper): Fixed button toggle
docs(stepper): Updated editable example
docs(stepper): Updated optional example
docs(stepper): Fixed overview example
docs(stepper): Updated Stepper Markdown
docs(stepper): Fixes stepper documentation
@raygervais raygervais force-pushed the stepper-documentation-update branch from b0c7a2a to 1bd9938 Compare April 3, 2018 23:25
@raygervais
Copy link
Contributor Author

Corrected E2E issues, should I add tests for the new examples?

@mmalerba
Copy link
Contributor

mmalerba commented Apr 3, 2018

@raygervais Our e2e coverage for most components is pretty lacking, but that's a separate issue. Not required for this PR.

@jelbourn jelbourn merged commit 6857426 into angular:master Apr 6, 2018
Longfld pushed a commit to Longfld/material2 that referenced this pull request Apr 6, 2018
)

Updates button logic setting button from true to !isLinear, including text indicator
of current state.
Creates editable and optional documentation examples
Imports into .MD file using special syntax
@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 Sep 8, 2019
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 target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs(stepper): add better documentation and examples
5 participants