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

Contrib guide: more restructuring #1461

Merged
merged 3 commits into from
Nov 3, 2024
Merged

Conversation

nedbat
Copy link
Member

@nedbat nedbat commented Oct 31, 2024

I'm continuing to look at the overall structure and move things around. Comments welcome. I wanted to flesh out the Code and Docs landing pages before making a real pull request, but maybe it's ok to merge this?


📚 Documentation preview 📚: https://cpython-devguide--1461.org.readthedocs.build/

@nedbat nedbat requested review from willingc and Mariatta October 31, 2024 13:21
Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thank you @nedbat!

Looks great. Two minor comments but don't hold merge on them:

  • Perhaps center or move the links into the titles of the landing page table
  • On the code contributions communications page, flip flop Discourse to come before mailing lists.

🎉

@@ -1,3 +1,5 @@
.. _c_code:
Copy link
Member

Choose a reason for hiding this comment

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

This made me think of C the language.

Are these c_ prefixes short for "contrib" and temporary whilst still a draft in parallel with the old devguide?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. I maybe should have explained that.

});
</script>
<style>
table.docutils td { vertical-align: top; }
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should move this to _static/devguide_overrides.css?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know about that file. I think tables should default to top-aligned, but idk if this will affect other things badly as a global setting.

*[I haven't adjusted the links in the table yet other than to add a link to the
major section at the top of each column.]*

.. list-table::
Copy link
Member

@hugovk hugovk Oct 31, 2024

Choose a reason for hiding this comment

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

I'm trying to think which is better, lists inside a table, or smaller cells, from an accessibility point of view, and I'm not sure :)

The new table of lists might be better, because there's no relation between the old cells that happen to be next to each other; that is, the old way was just for presentation and not structure. So I think the list might be better?

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Other people I've talked with definitely felt like the one-link-per-cell style of the current home page table is misleading because the rows have no meaning.

Comment on lines 108 to 109
[The contents had been listed below here. It was very long, and I don't think
it was helpful. The left sidebar has the contents.]
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I found it helpful: I often cmd+F on the homepage to find something, which won't work with a collapsed left sidebar.

It is a little unusual to have a large table-of-contents on a page, although it is out of the way at the bottom of the page, and not so big as to be a performance problem (like, we definitely wouldn't want the whole CPython TOC).

But it's not the end of the world to remove it if no one else wants it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fine to leave at the very bottom.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can leave it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I put it back, but not as deep as before, so it's not so long.

contrib/intro/index.rst Outdated Show resolved Hide resolved
@nedbat
Copy link
Member Author

nedbat commented Oct 31, 2024

@willingc the list has Repos, Discourse, Discord, Mailing lists, so I'm not sure what you are looking for.

@nedbat
Copy link
Member Author

nedbat commented Oct 31, 2024

@willingc I also don't understand this:

Perhaps center or move the links into the titles of the landing page table

@nedbat
Copy link
Member Author

nedbat commented Nov 1, 2024

Oh, "move the links into the titles," I see what you mean. I've changed it to this:
image

@nedbat nedbat force-pushed the nedbat/landing-pages branch from 4939b1e to d771c82 Compare November 1, 2024 12:04
@nedbat
Copy link
Member Author

nedbat commented Nov 1, 2024

And another option:

image

@willingc
Copy link
Collaborator

willingc commented Nov 2, 2024

@willingc the list has Repos, Discourse, Discord, Mailing lists, so I'm not sure what you are looking for.

I wanted to order the list to roughly reflect where conversations happen today. I wanted to de-emphasize the mailing lists relative to Discourse and Repos.

@nedbat nedbat marked this pull request as ready for review November 3, 2024 13:15
@nedbat nedbat merged commit e0bc90a into python:main Nov 3, 2024
4 checks passed
@nedbat nedbat deleted the nedbat/landing-pages branch November 3, 2024 13:18
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.

3 participants