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

Add components and hosts indexes #28

Merged
merged 13 commits into from
Sep 30, 2019
Merged

Add components and hosts indexes #28

merged 13 commits into from
Sep 30, 2019

Conversation

jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Sep 27, 2019

Adds /meetings-components and /meetings-hosts which list the meetings by component and host, respectively.

Other small changes:

  • previous_meetings is renamed to meetings
  • the page.component tag is changed to page.components so a meeting can be tagged as more than one component. PR 10823 is restored to mempool and policy (which is what it was listed under prior to Add pr page template and page variables #21

Resolves #25

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Great idea to enable multiple tags/components. Some suggestions below which might be out of scope. I came across some issues and proposed some fixes and improvements in #29 that builds on this commit.

meetings-hosts.html Outdated Show resolved Hide resolved
meetings-hosts.html Outdated Show resolved Hide resolved
meetings-components.html Outdated Show resolved Hide resolved
meetings-components.html Outdated Show resolved Hide resolved
meetings.html Outdated Show resolved Hide resolved
meetings.html Outdated Show resolved Hide resolved
_includes/linkers/meetings-pages.md Outdated Show resolved Hide resolved
@jnewbery
Copy link
Contributor Author

Thanks for the review @jonatack . I've taken all of your suggestions except changing the titles to "Previous meetings (by date|by Component|by host (in alphabetical order))". I think it looks a bit odd to have a title that says "by xx" and then have the three indexes below:

image

I've taken your commits and added a final commit that addresses your other comments.

One final question. I'm not sure whether 'meeting' is the right word here. Perhaps we should change it to be Previous PRs/Previous PRs by date/Previous PRs by component/ Previous PRs by host. Thoughts?

@jonatack
Copy link
Contributor

Good call on the titles. The titles idea came to me when the links weren't indicating which page was being viewed. Now that they do indicate it, the long titles were a bit much and confusing.

Meetings versus PRs: On the face of it, PRs seems good and would re-center the site around PRs. But then -- and maybe I'm overthinking it -- it could be somewhat confusing to squash the two together. Meetings and PRs are two separate domains, each with their own attributes. For example, the meaning of a date for meetings and PRs isn't the same, and meetings have hosts and participants, whereas PRs have authors and reviewers.

FWIW it might be an idea to add PR authors and created_at dates. As well as have the club auto-tweet/toot a selection of varying messages.

Back to the topic, if I were to design a database to model the business domains of the "club app", I'd surely want to have a meetings table and Meeting class domain model... idem for pull_requests, people (hosts, participants, PR authors, PR reviewers), tags with a pull_requests_tags join table, maybe even sponsor organisations for server costs and building a dev fund 😁

@jonatack
Copy link
Contributor

jonatack commented Sep 29, 2019

OTOH "PR reviews" could replace "meetings" while remaining distinct from PRs... it's a bit longer but reflects the name of the club. I don't have a strong opinion and empathise about not being sure.

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Tested ACK a4f7541 EDIT: found some issues, pushed various fixes in #31.

@jnewbery
Copy link
Contributor Author

Thanks Jon. I've taken all your commits from #31 except the final one. I didn't like padding the bottom of the page with lots of blank space. Instead I've added a css rule to highlight the target section when navigating to it. Let me know what you think.

@jonatack
Copy link
Contributor

Thanks, John. Tested ACK. Aqua is perhaps a bit electric on this otherwise sober-looking, black and white site. Perhaps try a shade of light grey, say, #ccc, #ddd, or #eee? For people like me who use the Dark Reader extension to convert everything in the browser to dark mode, a shade of gray can work well in dark mode, too. I tested aqua and the greys in both normal and dark mode. In dark mode with aqua, readability suffers a bit.

@jnewbery
Copy link
Contributor Author

Thanks John. Good idea with the color. I've changed it to #ddd. I think that looks better.

@jonatack
Copy link
Contributor

Tested ACK 7c9fa3a.

@jnewbery jnewbery merged commit eeab024 into master Sep 30, 2019
@jnewbery
Copy link
Contributor Author

Thanks Jon!

@jonatack jonatack mentioned this pull request Sep 30, 2019
2 tasks
@jnewbery
Copy link
Contributor Author

hmmm, I'm getting 404 for https://bitcoin-core-review-club.github.io/meetings-components/ (but it's working when I preview locally)

@jonatack
Copy link
Contributor

Same, is there an additional step needed for new pages on production deployment?

@jonatack
Copy link
Contributor

I'm seeing it for the meetings-components and meetings-hosts pages, e.g. the new ones.

@jnewbery jnewbery deleted the 2019-09-meetings-pages branch October 8, 2019 17:41
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.

Add component and host indexes
2 participants