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

Comprehensive documentation improvements #4031

Open
wants to merge 33 commits into
base: develop
Choose a base branch
from
Open

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Nov 19, 2024

Overview

This PR:

  • Adds a lot of new user-focused docs content to our docs guide
  • Cleans up a bunch of other stuff in the docs guide too
  • Makes changes in the UI to make some things easier to document and reference
  • Adds lots of small help bubbles in the UI, many of which link out to the new content in the docs guide
  • Adjusts terminology in the UI to make things more consistent and clear
  • Makes some small UX adjustments in a few places too

High level demo of docs content changes

2024-12-03_14-57-18.mp4

Demo of UI changes

2024-12-03_11-31-33.mp4

Notes

  • Docs: commits are docs content
  • Other commits are UI changes

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@seancolsen seancolsen added this to the v0.2.0 (beta release) milestone Nov 19, 2024
docs/docs/index.md Outdated Show resolved Hide resolved
docs/docs/user-guide/access-control.md Outdated Show resolved Hide resolved
docs/docs/user-guide/collaborators.md Outdated Show resolved Hide resolved
@seancolsen
Copy link
Contributor Author

seancolsen commented Nov 21, 2024

Thanks for the proofreading, @TheGiraffe3. I addressed your feedback in 71f92e8. (This commit will later be squashed, FYI, so you might not see it.)

@seancolsen seancolsen marked this pull request as ready for review December 3, 2024 20:28
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Dec 3, 2024
docs/docs/administration/backup-restore.md Show resolved Hide resolved
docs/docs/api/index.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mathemancer mathemancer left a comment

Choose a reason for hiding this comment

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

Thoughts while watching docs changes video

Big Picture: I like the new structure better than the old, with some caveats. I got distracted with some content concerns while watching, which are noted below (as well as structure caveats).

  • Administration should be below User Guide in nav as far as daily importance goes, but the "install" sections seem like they should be towards the top. This is a weakness of wrapping installation into broader section

  • Too much editorializing ("nascent and somewhat rough", "rudimentary support", etc.). Just ask for feedback (if anything). Perhaps a little section at the bottom of these pages inviting a user to request extra querying features, etc.

  • Overall, the prose is not very information-dense. We should spend some time tightening that up. For example, on the "Mathesar 0.2.0" page, you have:

    We do not support upgrading from previous versions to 0.2.0. You will need to install a fresh instance of Mathesar to use this version.
    Instead, consider:
    We do not support upgrading from previous versions to 0.2.0. You must reinstall Mathesar to use this version.

  • Generally, we should change the passive voice to active where possible. E.g., in the "Mathesar User Guide" section, the first sentence could start with "Mathesar gives you a spreadsheet-like...". If we really want to include the information about the fact that Mathesar is a web application, we could write "The Mathesar web application gives you a spreadsheet-like..." or even "The Mathesar web UI gives you a spreadsheet-like..."

  • I think "Collaborators" is a confusing term for the sidebar, since it's non-standard jargon. Maybe "Sharing PostgreSQL roles" for a header instead?

  • One of our most unique features is UI-driven schema (and data!!) migrations. This is not mentioned anywhere, or at least it's not visible in the nav bar anywhere

  • "Relationship", "Relational", "Related", etc. are overloaded terms, the way they're being used in the docs (and elsewhere). I.e., the docs overload them. A "Relation" in an RDBMS refers to a table, a view, a sequence, etc. Basically, anything whose OID is stored in the pg_class table in PostgreSQL. It does not refer to a foreign key or the mapping between two tables defined by entries in a foreign key column. This leads to never ending confusion. Consider the sentence from the PostgreSQL docs:

    Referential integrity
    A means of restricting data in one relation by a foreign key so that it must have matching data in another relation.

    Also see the definition of relation from the PostgreSQL Glossary:

    Relation
    The generic term for all objects in a database that have a name and a list of attributes defined in a specific order. Tables, sequences, views, foreign tables, materialized views, composite types, and indexes are all relations.

    More generically, a relation is a set of tuples; for example, the result of a query is also a relation.

    Normally, I detest appeals to authority (e.g., the authority of the PostgreSQL docs), but I think it's crucial to get this right, and align it with those docs, since:

    • An advanced user will reduce their estimation of our credibility if we misuse the term (especially since this is a kind of common "gotcha"-inducing mistake), and
    • A naive user will end up extremely confused if they're trying to cross-reference between Mathesar's docs (or UI) and the PostgreSQL docs when solving some problem.

    With all that said, I completely understand that the colloquial uses of the words "relation", "relationship", "related", etc. make it an easy term to use for foreign key references. I'd argue that's precisely the problem, since the colloquial meaning is a confusing red herring once you're reading other literature about RDBMSs (including the PostgreSQL docs).

    To soften my point a bit: While the PostgreSQL docs are quite precise w.r.t. the words "relation" and "relational" (these refer only to the "proper", technical concept), they're a bit looser with the words "relationship" and "related", which they treat as non-technical terms. We could go that route, but I think it would be a mistake, since it's really confusing to have such related words refer to such unrelated concepts. Even in the PostgreSQL docs this leads to some inconsistencies and confusion: See their section on parent-child relationships in the table inheritance. I also note that if you click through documentation versions for PostgreSQL in the Foreign key section, they seem to be moving away from "related" and "relationship" when discussing foreign key constraints.

My suggestion is that we try (yet again) to develop terminology for foreign key constraints that does not lean on "relationships" as a term, and maybe doesn't even lean on the concept. This will also have the benefit of letting us generalize to actually use the technical term "relation" when we have views, sequences, etc. represented in Mathesar.

Thoughts watching the product walkthrough video

  • "PostgreSQL Role Name": I definitely agree we need to change this away from "username", but why not just "role name"?
  • Connect/Read + Create : I think this is an improvement
  • Database Permissions page: Love the changes, definitely a net improvement
  • Same major concern about "relationship" terminology

Thoughts while reading through the code are noted line-by-line. I will probably submit another review with more line-by-line comments, but I'm a bit too fatigued at the moment to keep going on that.

docs/docs/user-guide/data-explorer.md Outdated Show resolved Hide resolved
docs/docs/user-guide/data-explorer.md Outdated Show resolved Hide resolved
docs/docs/user-guide/index.md Outdated Show resolved Hide resolved
docs/docs/user-guide/databases.md Outdated Show resolved Hide resolved
@@ -20,11 +28,86 @@ If you're starting your database from scratch with Mathesar you can either:

- Use another tool to create your database on an external server and then connect Mathesar to it. You can administer that external server yourself, or choose from a variety of hosted PostgreSQL solutions such as [Amazon RDS](https://aws.amazon.com/rds/postgresql/pricing/), [Google Cloud SQL](https://cloud.google.com/sql/postgresql), [Supabase](https://supabase.com/database), and others.

## Connecting a database
## Database Permissions {:#permissions}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using "Privileges" for consistency with the way they're referred to in the PostgreSQL docs, as well as these docs.

docs/docs/user-guide/metadata.md Outdated Show resolved Hide resolved
@@ -0,0 +1,190 @@
# Relationships
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed in main review.

@@ -0,0 +1,190 @@
# Relationships

Relationships allow a single cell in one table to reference a row in another table. When one table references another in this manner, the two tables are said to be "related". This is a core feature of relational databases, and it allows us to model complex data structures using multiple tables.
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph in particular mixes up the terminology in a way that may trip up users if they're doing any supplemental self-education.

docs/docs/user-guide/relationships.md Outdated Show resolved Hide resolved
docs/docs/user-guide/roles.md Outdated Show resolved Hide resolved
Copy link
Contributor

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

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

I think this is a significant improvement over the existing documentation and UI language. Really nice work! In this round of review I focused on the documentation. I think the overall hierarchy makes sense.

I did, however, run out of time, specifically for reviewing the "Your data in PostgreSQL" section and its documents. I'll take another look at those tomorrow but I wanted to make sure you had my existing comments first.

docs/docs/releases/0.1.4.md Show resolved Hide resolved
docs/docs/api/index.md Outdated Show resolved Hide resolved
docs/docs/api/index.md Outdated Show resolved Hide resolved
docs/docs/user-guide/access-control.md Show resolved Hide resolved
docs/docs/user-guide/access-control.md Outdated Show resolved Hide resolved
docs/docs/user-guide/roles.md Outdated Show resolved Hide resolved
docs/docs/user-guide/roles.md Show resolved Hide resolved
docs/docs/user-guide/schemas.md Outdated Show resolved Hide resolved
docs/docs/user-guide/stored-role-passwords.md Show resolved Hide resolved
docs/docs/user-guide/tables.md Outdated Show resolved Hide resolved
@seancolsen
Copy link
Contributor Author

@mathemancer I committed the changes from our pairing session via 4911484. I also made ed5d39d with another suggestion you had.

seancolsen and others added 4 commits December 11, 2024 10:49
Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com>
Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com>
Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com>
Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com>
@seancolsen
Copy link
Contributor Author

@zackkrida I think I've responded to all your feedback now. Everything you've suggested sounds great, with the exception of some comments I left on "Access Control" vs "Roles & Permissions". I implemented some of your suggestions. But for most of them, I've left them for you to implement. I'll leave this PR open for you to push changes. Let me know when you're satisfied.

And for any remaining content that you still have to review, I think it would be fastest for both of us if you could please push more commits directly. I can always look over your commits, which is faster for me than reading critique and applying changes. After seeing your critique thus far, I'm confident in the chances being very high that I'll agree with your changes. Just to reiterate: this "pile on" sort of process is not how we typically collaborate on code, but I think it's appropriate for docs content where (in my opinion) the need for speed sometimes outweighs the need for consensus. I also feel like, in your new role here, you ought to have more ownership over the docs content than anyone else on our team. So I would hope we can get to the point soon where we all defer to you on these docs decisions.

@zackkrida
Copy link
Contributor

@seancolsen that sounds good, I'll go ahead and push some commits later for the areas you've replied to with "Sounds good. Can you implement this?" and any of the documents I've yet to read.

Any hesitancy on my part to directly make changes was accounting for:

  • My relative newness to the project (It's my ~8th day as a maintainer!)
  • My absence from past discussions about terminology, conceptual models, etc.

I appreciate the reassurance that you think my direct edits should be okay!

The one area I'd like to push a bit for your insights on is this comment. It's one of those thoughts that opens a much bigger "can of worms" question about the approach to permissions that I don't feel I'm informed enough to weigh in on definitively. If, however, the concern there is a matter of your time / expediency, feel free to pass. I'm pretty content with leaving the existing language in place for now.

@zackkrida zackkrida self-assigned this Dec 12, 2024
@zackkrida
Copy link
Contributor

Leaving a note for myself: we should make sure it is clear in various places that the docker compose installation method is the "official" production installation method.

@seancolsen
Copy link
Contributor Author

@zackkrida If you could prioritize moving this forward, that would be awesome. It's been accumulating merge conflicts (I just resolved some). I have some work based on top of this PR already (#4104), and I'd like to stack even more on top of it soon.

@zackkrida
Copy link
Contributor

@seancolsen thanks for resolving those conflicts. I will be able to work on this tomorrow and expect to be able to get it into a "sufficient-to-merge" state fairly quickly.

@zackkrida zackkrida assigned seancolsen and unassigned zackkrida Jan 7, 2025
@zackkrida
Copy link
Contributor

@seancolsen I made two commits with some modest changes. I'm happy with the state of the PR but would appreciate your review and continued work to get it across the finish line.

  • 02ab36d contains small adjustments based on provided feedback in the review comments
  • aa1fc6c makes some modest changes to the "Databases" guide to improve flow and help (although in a simplistic manner that feels a bit like cheating) address the privileges vs. permissions issue present in the docs.

Feel free to revert or revise any of these changes as you see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants