Skip to content
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

feat(docs-infra): change navigation in resources page #34756

Open
wants to merge 3 commits into
base: master
from

Conversation

@ajitsinghkaler
Copy link
Contributor

ajitsinghkaler commented Jan 13, 2020

https://angular.io/resources needs to be structured to be able to navigate to all resources with improved user experience. A lone scroll bar in this page will not help the reader a great deal in exploring the resources
Fixes #33526

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Earlier there was no navigation available on the resources page

Issue Number: 33526

What is the new behavior?

Added navigation to the resources page

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@ajitsinghkaler ajitsinghkaler requested a review from angular/docs-infra as a code owner Jan 13, 2020
@googlebot googlebot added the cla: yes label Jan 13, 2020
@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Jan 13, 2020

@gkalpak can you please look into this.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 14, 2020

You can preview c771067 at https://pr34756-c771067.ngbuilds.io/.

@ngbot ngbot bot added this to the needsTriage milestone Jan 14, 2020
@gkalpak gkalpak added this to REVIEW in docs-infra Jan 14, 2020
Copy link
Member

gkalpak left a comment

Thx, @ajitsinghkaler! This looks great. A couple more tweaks and should be ready to merge (as far as I am concerned).

I think it now also makes sense to center the content under the tabs (because it looks weird to have the tabs centered and the content below left-aligned:

resources-uncentered

Note to self:
This should be approved by @fw-docs-marketing too (once ready).

@gkalpak gkalpak requested review from sjtrimble and angular/fw-docs-marketing Jan 14, 2020
@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Jan 15, 2020

@gkalpak should I make the content below center-aligned or should I make it take 100% of the width

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Jan 15, 2020

I would preserve the current width (unless @sjtrimble feels otherwise).

@ajitsinghkaler ajitsinghkaler force-pushed the ajitsinghkaler:recource-list-nav-change branch 2 times, most recently from b75cf12 to 4a4f67f Jan 15, 2020
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 15, 2020

@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Jan 15, 2020

@gkalpak I've made the changes can you please review this. Should I also add tests the tests that are there in the contributor's page regarding location

Copy link
Member

gkalpak left a comment

Thx, @ajitsinghkaler 👍 Yes, please, add some tests for the new functionality (i.e. toggling between categories, showing entries from a specific category only and interacting with the URL query).

I have left some comments to do some clean-up (which is not directly related to this PR, but would be awesome if you could do (on a separate commit)).

Other than that, this lgtm (pending the addition of tests) 👍

…page

In resources component there were unused functions removed unused funtions
https://angular.io/resources needs to be sturctured to be able to navigate to all resources with improved user experience. A lone scroll bar in this page will not help the reader a great deal in exploring the resources
Fixes #33526
@ajitsinghkaler ajitsinghkaler force-pushed the ajitsinghkaler:recource-list-nav-change branch from 4a4f67f to 91ea8d6 Jan 15, 2020
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 15, 2020

@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Jan 15, 2020

@gkalpak I've added the cleanup please have a look and can you please tell me why the location service test //// is failing

Copy link
Member

gkalpak left a comment

The tests are failing, because the injector that is set up for the tests must be updated to provide the necessary dependencies here. I.e. remove PlatformLocation and provide a mock Location service (you can use this one).

Don't forget to add tests for the new functionality (as mentioned above).

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 16, 2020

@ajitsinghkaler ajitsinghkaler force-pushed the ajitsinghkaler:recource-list-nav-change branch from 42c2efc to 111f54d Jan 16, 2020
@ajitsinghkaler ajitsinghkaler changed the title fix(docs-infra): change navigation in resources page feat(docs-infra): change navigation in resources page Jan 16, 2020
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 16, 2020

@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Jan 16, 2020

@gkalpak added the tests please have alook

Copy link
Member

gkalpak left a comment

Almost there 😃
I left some minor clean-up suggestions for the tests. Other than that, you just need to get CI green and this is good to go.

added tests for new navigation that is added o the resources page

#Fixes #33526
@ajitsinghkaler ajitsinghkaler force-pushed the ajitsinghkaler:recource-list-nav-change branch from 111f54d to 1a05786 Jan 18, 2020
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 18, 2020

@ajitsinghkaler

This comment has been minimized.

Copy link
Contributor Author

ajitsinghkaler commented Jan 18, 2020

@gkalpak I've made all the changes. @sjtrimble can you please review my pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.