Skip to content

Conversation

@amcdnl
Copy link
Contributor

@amcdnl amcdnl commented Oct 11, 2017

This PR:

  • Adds type column for properties
  • Adds null check to descriptions

@amcdnl amcdnl self-assigned this Oct 11, 2017
@amcdnl amcdnl requested a review from jelbourn October 11, 2017 14:18
@amcdnl amcdnl requested a review from devversion as a code owner October 11, 2017 14:18
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 11, 2017
@jelbourn
Copy link
Member

Could you include a screenshot of how this looks? How does it fit on mobile?

@amcdnl
Copy link
Contributor Author

amcdnl commented Oct 11, 2017

Here is a before/after

img

@jelbourn
Copy link
Member

I'm not a huge fan of how much horizontal space it takes up now; any ideas around how we can keep the name and the type in the same vertical space next to the description?

@willshowell
Copy link
Contributor

From angular/material.angular.io#287 (comment), would you consider moving the type to the description cell and making it distinct via font weight and padding? Just instead of "Return Value:", have it sat "Type:"

screen shot 2017-10-10 at 11 11 19 am

@jelbourn
Copy link
Member

That could work- I do want to try emphasizing the type by using a monospace font with a different color rather than writing out type to see how that comes across

@amcdnl
Copy link
Contributor Author

amcdnl commented Oct 16, 2017

Here is the updated look:

img

It requires: angular/material.angular.io#292

<td class="docs-api-property-description">{$ property.description | marked | safe $}</td>
<td class="docs-api-property-description">
<div class="docs-api-property-type">
Type: <code>{$ property.type $}</code>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of having "Type: " explicitly. Repeated text like this creates a lot of visual noise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Should I just remove type and leave it like is otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Can you post a SS of what that looks like?

@amcdnl
Copy link
Contributor Author

amcdnl commented Oct 24, 2017

I'm not sure I like this:

img

@jelbourn
Copy link
Member

What if we add it line with the name, TypeScript syntax style?

@Input()
disabled: boolean

@amcdnl
Copy link
Contributor Author

amcdnl commented Oct 25, 2017

@jelbourn - That doesn't look bad ->

img

@jelbourn
Copy link
Member

I do like that more

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

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 25, 2017
@mmalerba mmalerba merged commit 8993451 into angular:master Oct 27, 2017
mmalerba added a commit that referenced this pull request Oct 27, 2017
mmalerba added a commit that referenced this pull request Oct 27, 2017
@jelbourn
Copy link
Member

@amcdnl can you re-create this PR? It was merged before final changes could be made

@amcdnl
Copy link
Contributor Author

amcdnl commented Oct 30, 2017

Done in #8125

@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 7, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants