Skip to content

Conversation

@sabine
Copy link
Collaborator

@sabine sabine commented Mar 21, 2023

This patch

  • renames package urls and handlers to better reflect their purpose and be more consistently named
  • introduces dedicated url and handler for package files (README, CHANGELOG, LICENSE)
  • moves all non-handler package-related functions into a module PackageHelpers to prevent confusion with handlers
  • extends Ocamlorg_frontend.Package by homepages and source

Resolves #995.

There's more potential for tidying up / clarifying that this patch does not include, e.g.

  1. sidebar state of package_documentation handler
  2. extend voodoo to inspect for README, CHANGELOG, LICENSE and add info about these files to package.json
  3. make voodoo emit documentation_status as part of package.json
  4. after 2+3) make Ocamlorg_frontend.Package pick up this info from package.json

@sabine sabine force-pushed the refactor_package_routes_handler_urls branch from d4192e8 to be75cf4 Compare March 22, 2023 12:58
Copy link
Collaborator

@cuihtlauac cuihtlauac left a comment

Choose a reason for hiding this comment

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

I've uploaded a branch with some changes. Nothing significant, just helped me review. Feel free to pick and choose.

https://github.com/ocaml/ocaml.org/tree/cuihtlauac-999

It may have been possible to split this in several PR.

In url.ml I'm not so convinced by the names:

package_redirect
package_docs_redirect 

To me, redirection is an implementation option which is not relevant to URL string processing and should not be exposed. Maybe package_overview_without_version and package_documentation_without_version would be better. I'm not sure.

Overall, it goes in the right direction, this is what matters.

@sabine
Copy link
Collaborator Author

sabine commented Mar 22, 2023

In url.ml I'm not so convinced by the names:
package_redirect
package_docs_redirect
To me, redirection is an implementation option which is not relevant to URL string processing and should not be exposed. Maybe package_overview_without_version and package_documentation_without_version would be better. I'm not sure.

I'm pretty sure I don't want to name it in such a way that someone will be tempted to use that URL in a template. I considered moving it to the redirection.ml file, as it's just a redirect. However, there were no "dynamic redirects" happening in that file and that discouraged me. Now that I think about it again, I think I should move the redirects to redirection.ml.

@cuihtlauac
Copy link
Collaborator

Now that I think about it again, I think I should move the redirects to redirection.ml.

I had the same thought. Let's do that and remove them from Url

@sabine sabine merged commit 3160b61 into ocaml:main Mar 22, 2023
@sabine sabine deleted the refactor_package_routes_handler_urls branch March 22, 2023 15:05
Zineb-Ada pushed a commit to Zineb-Ada/ocaml.org that referenced this pull request Apr 3, 2023
* refactor package urls, routes, and handlers
* move package redirects to redirection.ml
Zineb-Ada pushed a commit to Zineb-Ada/ocaml.org that referenced this pull request Apr 4, 2023
* refactor package urls, routes, and handlers
* move package redirects to redirection.ml
cuihtlauac added a commit that referenced this pull request Apr 6, 2023
* make package documentation link more obvious (#828)

* make documentation link more obvious

* minor style improvement

Co-Authored-By: Sabine Schmaltz <sabine@tarides.com>

* improve authors/maintainers display on package overview (#1001)

* refactor package urls, routes, and handlers (#999)

* refactor package urls, routes, and handlers
* move package redirects to redirection.ml

* Unify package overview and documentation layout (#1015)

* unified package overview/docs layout

Rearranges package overview and documentation such that:

* package_layout.eml defines the two sidebars and the
  content area
* there is a navigation element to switch between
  Overview (About) and Documentation (Docs)
* there is a placeholder element for the upcoming
  in-package search (to remind us how important
  this feature is and to show users that
  we have a plan where it goes)

Consequences:

* package overview page now has a collapsing sidebar
  with a button to slide it in on small screens
* package documentation now has a tablet (md) layout
  that shows the sidebar on-screen, instead of collapsed

* add text-sm which was lost during unification

* "Overview" instead of "About" since we have the space

* Add support for sitemap.xml

* Generate sitemap.xml by dream

* Apply suggestions from @cuihtlauac code review

Co-authored-by: Cuihtlauac Alvarado <cuihtlauac@users.noreply.github.com>

* Convert lists of urls to sequences and change names of functions

* Apply suggestions from @cuihtlauac code review 2

Co-authored-by: Cuihtlauac Alvarado <cuihtlauac@users.noreply.github.com>

* Delete subdomains URLs and URLs returning status code other than 200

* Apply suggestions from @cuihtlauac code review 3

Co-authored-by: Cuihtlauac Alvarado <cuihtlauac@users.noreply.github.com>

* Update PR

* formating

* do not touch playground asset

* Apply suggestions from @cuihtlauac code review 4

* Apply suggestions from @cuihtlauac code review 5

* Apply suggestions from @cuihtlauac code review 6

* Apply suggestions from @cuihtlauac code review 7

* Apply suggestions from @cuihtlauac code review 8

---------

Co-authored-by: Sabine Schmaltz <sabine@tarides.com>
Co-authored-by: sabine <sabine@users.noreply.github.com>
Co-authored-by: Cuihtlauac Alvarado <cuihtlauac@users.noreply.github.com>
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.

Dream errors poping-up in logs

2 participants