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

Modernised nltk.org website #18

Closed
wants to merge 2 commits into from
Closed

Modernised nltk.org website #18

wants to merge 2 commits into from

Conversation

tomaarsen
Copy link
Member

@tomaarsen tomaarsen commented Oct 6, 2021

Hello!

Pull request overview

  • Modernised the nltk.org website by modifying the Sphinx theme to nltk_theme, which is based on @autophagy's Insegel.
  • Moved the team information from contribute.rst into a separate file called team.rst.
  • Renamed items on the Table of Contents menu.

Have a look at this website right here: https://tomaarsen.github.io/nltk.github.com
This is based on https://github.com/tomaarsen/nltk.github.com/tree/modernised, which is identical to the branch I'm trying to merge here. With other words, the website linked above is what you'll get upon merging.

See nltk/nltk#2845 for the documentation of this PR.

Additional Info

The website here was built for version 3.6.3, and it links to the 3.6.3 tag of NLTK in the footer. I would also recommend quickly finishing up NLTK 3.6.5 (perhaps I can help), after which we can create a new documentation version in a smoother fashion. To generate this documentation for version 3.6.3, I had to checkout 3.6.3 and copy over the web folder from my PR in NLTK - definitely not ideal. That will improve drastically once nltk/nltk#2845 is merged, and we start releasing new versions.

  • Tom Aarsen

@dannysepler
Copy link

dannysepler commented Oct 9, 2021

finally got around to looking at this. it is BEAUTIFUL!!!!! Consider this an "Approval" since I don't seem to have the approve button available.

Here's what I especially like.

  • The color is really pleasing to the eye
  • The font is AWESOME and very modern looking
  • It's a lot more spaced-out, compared to the current site which can feel overwhelming on certain pages
  • Having links on the left side feels a lot more familiar to me than the right
  • The whole design of the thing. Really great choice of theme!!

here are a few notes i have, but i'm a little rusty with web dev and happy to do it myself. i'd somewhat prefer merging as is considering this PR is already 170k lines and picking these sorts of things up as followups.

  • we may want to put a facivon icon on the browser tab. Then again, the old site didn't have this either
  • when i make the website narrow (ie a mobile view) the hamburger menu is on the bottom right. it should be on the top left
  • i see at least one instance of "isort:skip_file" rendered in the html (on the "html reference" link)
  • i would maybe remove the "." at the end of "Search."

@tomaarsen
Copy link
Member Author

I'm very happy to see the positive response!

Regarding the possible changes:

  • To put a favicon, we would need a favicon. That would require a logo of sorts, or perhaps just an N. I didn't want to commit to anything this big for now.
  • This is definitely an issue. I've fixed it now in tomaarsen/nltk_theme@9a13777. This theme will be used when building new documentation from nltk/nltk directly. With other words, this fix in nltk_theme will affect future builds of the website. (That said, it's not been changed for this PR, but I'm not sure whether it's best to even merge this PR. We might just want to update nltk/nltk, merge my PR there, and build new documentation for e.g. NLTK 3.6.5, instead of using this PR to create a website for 3.6.3.)
  • I don't really know how to deal with this. We use Sphinx, which automatically generates the contents of our pages for us - it just takes it directly from the source. I just supply a theme for it to fill. I could perhaps filter this stuff away, but it would be a lot of work.
  • And yes, I agree. "Search..." just seems a bit weird, It's "Search" now (I also changed this in tomaarsen/nltk_theme@9a13777)

Thanks for the detailed feedback! It's been very useful.

  • Tom Aarsen

@stevenbird
Copy link
Member

Great to see this feedback!

Unfortunately we don't have a favicon.
Yes, we'll merge nltk/nltk#2845 then rebuild the website from there.

@tomaarsen
Copy link
Member Author

Merged via 35f03a1. Glad to see the website up!

@tomaarsen tomaarsen closed this Oct 11, 2021
@tomaarsen tomaarsen deleted the modernised branch October 11, 2021 07:23
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

3 participants