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

Remove unused layouts and their CSS. #2624

Merged
merged 3 commits into from
Oct 16, 2019
Merged

Remove unused layouts and their CSS. #2624

merged 3 commits into from
Oct 16, 2019

Conversation

XhmikosR
Copy link
Contributor

  • announcements.hbs
  • in-the-news.hbs

I left the blogAnnounce collection as is, although it probably is redundant too, so must be its feed

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Sep 29, 2019

Actually now that I see site.json it seems the feed is also unused. Let me know how to proceed.

"feeds": [
{
"link": "feed/blog.xml",
"text": "Node.js Blog"
},
{
"link": "feed/releases.xml",
"text": "Node.js Blog: Releases"
},
{
"link": "feed/vulnerability.xml",
"text": "Node.js Blog: Vulnerability Reports"
}
],

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 1, 2019

@Trott should I also remove completely announcements?

@Trott
Copy link
Member

Trott commented Oct 1, 2019

@Trott should I also remove completely announcements?

I'm not sure. I don't know the ramifications. We can ping more widely for review or I can be educated.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 1, 2019

Let's ask more people. I added the complete removal of announcements. Not sure if we should add any redirections or not.

@Trott
Copy link
Member

Trott commented Oct 2, 2019

@nodejs/website

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 14, 2019

@nodejs/build should we redirect the announcements index file or their feed? Note that this actually only removes the /announcements/index.html/ file and announcements.xml

Otherwise, I will split this patch so that we can merge the first one which is safe.

@XhmikosR
Copy link
Contributor Author

Since I'm not getting any reviews, I dropped the second patch from this PR.

Now the patch just removes unused layouts and their CSS.

@XhmikosR XhmikosR marked this pull request as ready for review October 16, 2019 07:36
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

This seems fine to me.
I'm a little shocked that we have nobody showing up to review though? what happened to our website team and where is our shiny new website that we've had a whole team pouring over?

* announcements.hbs
* in-the-news.hbs
@XhmikosR XhmikosR changed the title Remove unused layouts. Remove unused layouts and their CSS. Oct 16, 2019
@alexandrtovmach
Copy link
Contributor

I think we haven't somebody who can give an answer about removed layouts, so let's merge PR and take a look on any issues that can be produced

@XhmikosR
Copy link
Contributor Author

This shouldn't affect anything. I moved the other patch to separate PRs.

Copy link
Member

@nschonni nschonni left a comment

Choose a reason for hiding this comment

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

Not sure about removing the content (now in other PR), but these do look unreferenced anyway so 👍

@nschonni nschonni merged commit 682d82c into nodejs:master Oct 16, 2019
@bnb
Copy link
Contributor

bnb commented Oct 16, 2019

I'm a little shocked that we have nobody showing up to review though? what happened to our website team and where is our shiny new website that we've had a whole team pouring over?

@rvagg you can find that work in nodejs/nodejs.dev.

@rvagg
Copy link
Member

rvagg commented Oct 16, 2019

@bnb thanks, so when do we get to replace nodejs.org?

@bnb
Copy link
Contributor

bnb commented Oct 16, 2019

That's a great question. I'm not involved to the point to be able to assert an answer to it. Perhaps @amiller-gh could give some insight on, though I don't necessarily think this PR is the correct venue for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants