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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

A load of updates #262

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@MattIPv4
Copy link
Member

MattIPv4 commented Jan 1, 2019

  • Resolves #152 [SRI support on search] (includes, closes #251)
  • Resolves #92 [Missing robots.txt]
  • Resolves #258 [GitHub banner issues]
  • Resolves #143 [BreadcrumbList implementation] (includes modified, closes #178)
  • Resolves #255 [Throng dep vuln] (includes, closes #256)
  • Resolves #18 [Mobile issues]

Additionally:

  • Update usage stats on about
  • Add me (MattIPv4) to about
  • Overhaul home (responsive, some key info added)
  • Fix brand consistency
  • Re-order navigation to prioritise browsing libraries
  • Fix bad footer margin

Happy New Year! Let's get cdnjs off to a great start in 2019 馃帀

@MattIPv4 MattIPv4 referenced this pull request Jan 1, 2019

Open

Mobile issue #18

@MattIPv4 MattIPv4 added the enhancement label Jan 1, 2019

MattIPv4 added some commits Jan 1, 2019

@MattIPv4 MattIPv4 changed the title [WIP] A load of updates [Website] A load of updates Jan 1, 2019

@MattIPv4 MattIPv4 requested a review from PeterDaveHello Jan 1, 2019

@PeterDaveHello
Copy link
Member

PeterDaveHello left a comment

Let's separate different topics into different pull requests. It's not a good idea to mix them all.

@MattIPv4

This comment has been minimized.

Copy link
Member

MattIPv4 commented Jan 1, 2019

@PeterDaveHello How would you suggest splitting this all up then? Quite a bit of it is intertwined and would likely cause conflicts being split up

@PeterDaveHello

This comment has been minimized.

Copy link
Member

PeterDaveHello commented Jan 1, 2019

@MattIPv4 One topic, one PR. Conflucts are possible, so there mey be rebasing and conflicts resolving before getting merged.

@MattIPv4

This comment has been minimized.

Copy link
Member

MattIPv4 commented Jan 1, 2019

@PeterDaveHello Is there a reason for splitting it up into different PRs though? The commits are split up per change so everything is pretty easily traceable.

Splitting everything up just makes a load of extra work and issues that frankly I don't want to be dealing with.

@PeterDaveHello

This comment has been minimized.

Copy link
Member

PeterDaveHello commented Jan 1, 2019

@MattIPv4 It's hard to review, and revise.

I guess the reason for extra works is trying to do many things one time, which is not needed.

@MattIPv4

This comment has been minimized.

Copy link
Member

MattIPv4 commented Jan 1, 2019

@PeterDaveHello The reason I've packed everything in at once is because some of these changes have been sitting in stale PRs or issues for a long time and I'd rather have them all in one PR so they can just be actually dealt with.

@PeterDaveHello

This comment has been minimized.

Copy link
Member

PeterDaveHello commented Jan 1, 2019

When I need to review a pull request, I need to review the diff of codes, the outcomes, I don't think it's a good idea to cherrypick every commit in it, instead, per issue per pull request, which means per branch to review, that I think it's a good idea.

@PeterDaveHello

This comment has been minimized.

Copy link
Member

PeterDaveHello commented Jan 1, 2019

Isn't about stale or not, you can still pick or revise staling PR separately.

@MattIPv4

This comment has been minimized.

Copy link
Member

MattIPv4 commented Jan 1, 2019

I really don't have the time or energy to go through all the changes I've made to better cdnjs here again and split them up and then resolve all the conflicts that I've already resolve once here.

If you or someone else want to, feel free, or this PR can just chill like all the others here with no progress.

@PeterDaveHello

This comment has been minimized.

Copy link
Member

PeterDaveHello commented Jan 1, 2019

@MattIPv4 I'd do it when I have time, but the same here, a proper peer review process won't cost less time or energy than doing the changes, so mixing them up increases the workload of reviewing, with all due respect.

@PeterDaveHello

This comment has been minimized.

Copy link
Member

PeterDaveHello commented Jan 1, 2019

@MattIPv4 btw, the review process itself, do you have suggestion about not cherrypick every single commit of them for this PR but can certainly be sure each one works well? I don't know yet. These changes seem not testable to me as a reviewer :(

@MattIPv4

This comment has been minimized.

Copy link
Member

MattIPv4 commented Jan 1, 2019

@PeterDaveHello If I was reviewing this, I would spool up the local web server with the changes present and then work through the list I provided at the top of what should be changed and fixed and verify that it's all okay. At the same time just check for anything that is obviously broken due to these changes.

@PeterDaveHello

This comment has been minimized.

Copy link
Member

PeterDaveHello commented Jan 1, 2019

@MattIPv4 thanks, but this method can't deal with the interaction between different changes, and it's also not strict enough.

I think #261 is something good to follow, same topic, different steps/commits.

@MattIPv4

This comment has been minimized.

Copy link
Member

MattIPv4 commented Jan 2, 2019

Breakdown for new PRs

  • Resolves #152 [SRI support on search] (includes, closes #251)
  • INFO Resolves #92 [Missing robots.txt]
  • RESPONSIVENESS Resolves #258 [GitHub banner issues]
  • RESPONSIVENESS Resolves #143 [BreadcrumbList implementation] (includes modified, closes #178)
  • Resolves #255 [Throng dep vuln] (includes, closes #256)
  • RESPONSIVENESS Resolves #18 [Mobile issues]

Additionally:

  • INFO Update usage stats on about
  • INFO Add me (MattIPv4) to about
  • INFO/RESPONSIVENESS Overhaul home (responsive, some key info added)
  • INFO Fix brand consistency
  • RESPONSIVENESS Re-order navigation to prioritise browsing libraries
  • RESPONSIVENESS Fix bad footer margin

Happy New Year! Let's get cdnjs off to a great start in 2019 馃帀

@MattIPv4 MattIPv4 changed the title [Website] A load of updates A load of updates Jan 2, 2019

@MattIPv4 MattIPv4 referenced this pull request Jan 3, 2019

Closed

GitHub corner banner #258

@Glennmen

This comment has been minimized.

Copy link

Glennmen commented Jan 3, 2019

@MattIPv4 What is the use of this PR... You should never combine different features/improvements into a single PR.

Anyways my code shouldn't be in here, there already is a PR for it and @PeterDaveHello will review it once he has the time.

@PeterDaveHello

This comment has been minimized.

Copy link
Member

PeterDaveHello commented Jan 3, 2019

@Glennmen sorry for the delay again and thanks for your understanding

@MattIPv4

This comment has been minimized.

Copy link
Member

MattIPv4 commented Jan 3, 2019

@Glennmen Hi, the reason behind this PR was fixing quite a few stale issues and PRs to get them sorted for the new year. Previous projects I've worked on were fine with mega PRs, evidently not so here. This PR is in the process of being split up and I have pinged @PeterDaveHello in the relevant PRs.

@MattIPv4 MattIPv4 closed this Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment