Skip to content

feat(aio): api pages styling - #16327

Merged
petebacondarwin merged 1 commit into
angular:masterfrom
sjtrimble:api-page-styles-2
Apr 26, 2017
Merged

feat(aio): api pages styling#16327
petebacondarwin merged 1 commit into
angular:masterfrom
sjtrimble:api-page-styles-2

Conversation

@sjtrimble

@sjtrimble sjtrimble commented Apr 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Banner class code consolidation for API pages
  • Set up temporary table of contents class and file
  • API pages title styling
  • Add color styling to doc-type listed on each API details page
  • Classes and SCSS variables refactor
  • Mobile optimization on headers, info-banner, and API pages
  • API page table custom styling
  • API Class Overview template code overview into table format

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@sjtrimble sjtrimble self-assigned this Apr 25, 2017
@mary-poppins

mary-poppins commented Apr 25, 2017

Copy link
Copy Markdown

The angular.io preview for f18c6d70b0f074f941599a52db3e9a7c6c1ec702 is available here.

gkalpak
gkalpak suggested changes Apr 25, 2017
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM except for a couple of minor issue.

Comment thread aio/src/styles/2-modules/_api-list.scss Outdated

@gkalpak gkalpak Apr 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This messes the search icon in the API docs search field
(I wonder what this style is for anyway.)

Comment thread aio/src/app/app.component.html Outdated

@gkalpak gkalpak Apr 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This (and the corresponding CSS) seems unrelated to this PR.

@sjtrimble sjtrimble Apr 25, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is intended to be the starting point of the placement for the fixed TOC but I haven't had much luck with the 'fixed' part of it ...

Do you think it's okay to leave for now, or should I remove the HTML and rename the PR to include this. I did include the description of this change in the PR description/comments.

@gkalpak gkalpak Apr 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generally it is not a good idea to leave dead code in (because often it ends up staying there forever), but if you are confident that you are going to work on it in the near future, I don't mind 😃

@mary-poppins

mary-poppins commented Apr 25, 2017

Copy link
Copy Markdown

The angular.io preview for 7303d095929f144c7eb52c48e15ffa83c63a4b7a is available here.

@sjtrimble sjtrimble mentioned this pull request Apr 25, 2017
Closed
@mary-poppins

mary-poppins commented Apr 25, 2017

Copy link
Copy Markdown

The angular.io preview for 8edf90902970bf3e805fb82bf17e33fef0708e75 is available here.

@mary-poppins

mary-poppins commented Apr 25, 2017

Copy link
Copy Markdown

The angular.io preview for 853e1ca2af029b06b47a88e9cd468e1700e6ba30 is available here.

@sjtrimble sjtrimble changed the title WIP feat(aio): api pages styling feat(aio): api pages styling Apr 26, 2017
@sjtrimble sjtrimble added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Apr 26, 2017
@mary-poppins

mary-poppins commented Apr 26, 2017

Copy link
Copy Markdown

The angular.io preview for dacea81a609f7359b842fbfc34889ac4231ab5c5 is available here.

gkalpak
gkalpak approved these changes Apr 26, 2017
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My understanding is that the API pages are still WIP (i.e. this is not the final PR for API pages), so there is little point in nitpicking on every (visual or other) issue atm.

In that sense, this LGTM. This should get in before it starts growing too big 😃

@petebacondarwin petebacondarwin mentioned this pull request Apr 26, 2017
Closed
@petebacondarwin

petebacondarwin commented Apr 26, 2017

Copy link
Copy Markdown
Contributor

I agree, we just need to fix the conflicts

@sjtrimble @petebacondarwin
feat(aio): api pages styling
95dffad 
- Banner class code consolidation for API pages
- Set up temporary table of contents class and file
- API pages title styling
- Add color styling to doc-type listed on each API details page
- Classes and SCSS variables refactor
- Mobile optimization on headers, info-banner, and API pages
- API page table custom styling
- API Class Overview template code overview into table format
@petebacondarwin

petebacondarwin commented Apr 26, 2017

Copy link
Copy Markdown
Contributor

I rebased and force pushed

@mary-poppins

mary-poppins commented Apr 26, 2017

Copy link
Copy Markdown

The angular.io preview for 95dffad is available here.

@petebacondarwin
petebacondarwin merged commit bb52e22 into angular:master Apr 26, 2017
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 26, 2017
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 11, 2019

Copy link
Copy Markdown

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 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@gkalpak gkalpak gkalpak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@sjtrimble sjtrimble

Labels

action: merge The PR is ready for merge by the caretaker cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sjtrimble @mary-poppins @petebacondarwin @gkalpak @googlebot