Skip to content

docs(toolbar): spruce up the toolbar component examples #18745

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
merged 8 commits into from
May 1, 2020

Conversation

livvielin
Copy link
Contributor

@livvielin livvielin commented Mar 7, 2020

Screenshot of basic, colored, and multi-row toolbars included in toolbar-overview-example:
Screen Shot 2020-03-20 at 10 31 11 AM

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 7, 2020
@devversion devversion self-requested a review March 7, 2020 09:10
@livvielin livvielin marked this pull request as ready for review March 9, 2020 16:56
@livvielin livvielin requested a review from jelbourn as a code owner March 9, 2020 16:56
@livvielin
Copy link
Contributor Author

Looking to get initial feedback on this. Is this the sort of thing you're looking for to spruce examples up? Should there be more styling or complex interactions? I wasn't sure since the demo pages seem to start with "Basic usage" on the overview page and keep the fancier examples in the demo section.

@jelbourn jelbourn added the G This is is related to a Google internal issue label Mar 9, 2020
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

This is definitely a step in the right direction! I would continue to expand the overview here with some additional examples that showcase a few more features. I think the end result would be something like

  • toolbar-overview-example - what you have now, plus an example of changing the color, and an example with two rows.
  • toolbar-basic-example - what you have now by itself
  • toolbar-multirow-example - no changes from what's there

The goal is to make it more immediately clear what the component can do the very first time someone lands on the overview, while still have more discrete examples for specific use-cases.

One other thing I would add to this PR: add your examples to the toolbar demo page in the dev-app. That will make it easy for someone on the team to pull down your change and test it out. Here's an example of what I mean: 20b7c77

It would also be great if you could paste a screenshot of the example in the PR description (GitHub lets you paste images directly into the description)

@livvielin
Copy link
Contributor Author

Awesome, thanks for the feedback! I've added a couple more examples to toolbar-overview-example and then added it to the toolbar demo page in dev-app.

@livvielin livvielin changed the title Spruce up the basic toolbar example Spruce up the toolbar component examples Mar 20, 2020
@livvielin livvielin changed the title Spruce up the toolbar component examples docs(toolbar): spruce up the toolbar component examples Mar 27, 2020
@livvielin
Copy link
Contributor Author

@jelbourn Is there anything else I should add to this PR or change?

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, just one small nit

(sorry for the delayed review, this accidentally fell off my radar)

@jelbourn jelbourn added lgtm target: patch This PR is targeted for the next patch release docs This issue is related to documentation cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Apr 23, 2020
@jelbourn jelbourn added merge safe action: merge The PR is ready for merge by the caretaker 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 Apr 24, 2020
@jelbourn jelbourn added target: patch This PR is targeted for the next patch release target: minor This PR is targeted for the next minor release and removed target: minor This PR is targeted for the next minor release target: patch This PR is targeted for the next patch release labels Apr 29, 2020
@jelbourn jelbourn merged commit 435de94 into angular:master May 1, 2020
@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 Jun 1, 2020
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 G This is is related to a Google internal issue 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