If this document is meant to be the entrypoint for developers, having
the supported workflow table the second item is very distracting; so move it
to the bottom where more relevant information is presented first.
Depends on D4967
Details
Details
- Reviewers
ashoat varun - Commits
- rCOMMcfbeb52b0aba: [Docs] Update supported workflow information
View rendered markdown:
https://github.com/CommE2E/comm/blob/jonringer/nix-dev-docs/docs/nix_dev_env.md
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Disagree, it should be at the top, it's crucial information. The reader should be able to drop out of the flow if it's not useful for them. Very frustrating to go through everything only to get to the end and find out it doesn't work for them
If/when you move it to the top, if this diff is just flipping Android / iOS and D4967 has been landed, feel free to remove me from the diff to make it appear accepted
| docs/nix_dev_env.md | ||
|---|---|---|
| 80–81 ↗ | (On Diff #16033) | Why are these green now? |
Comment Actions
Disagree, it should be at the top, it's crucial information.
Alright, I'll just add a comment that nix can get you most of the way, but some workflows still require some manual steps.
| docs/nix_dev_env.md | ||
|---|---|---|
| 80–81 ↗ | (On Diff #16033) | If the user follows the prerequisite sections, then they should be supported. Future nix work can be done to reduce the number of steps (e.g. flipper, idb, reactotron). But it's pretty close to it will ever be to a fully nix environment. |
Comment Actions
Requesting changes for the inline edits, the footnote discussion can be a Linear issue or separate diff or something. Also curious for @ashoat's thoughts when he gets back, if he decides the footnotes are fine then it's ok. So maybe a Linear issue should be created, he should be subscribed, so when he sees it there he can respond!
| docs/nix_dev_env.md | ||
|---|---|---|
| 15–18 ↗ | (On Diff #16081) | At this point, nearly every workflow requires additional steps. Maybe we can think about a way to make this seem better in the future -- users should know before reading the footnote that Nix isn't going to do everything for them. But, it will do most of the work for them, and anything else will be smaller. While the user might still know this, the footnote isn't really helping right now. It makes it seem like we told them Nix will do a lot of things, and then later slid in that they will have to do more things. Currently, the footnote for these sections has a lot of examples for "additional steps." Plus, "additional steps" is broad and could mean any amount of steps, especially to a user who has no familiarity with the dev environment. So we should think about that, since most of the time footnotes should be used sparingly. If they end up affecting a large portion of a section, we should just make it information the user knows outside of just the footnote. But this can be a separate issue. |
| 22 ↗ | (On Diff #16081) | I don't think we need the "optional," that can be made clearer in the actual documentation for React/Redux Dev Tools. Here, in a footnote, it's not as necessary. |
| 22 ↗ | (On Diff #16081) | Should this be "the" Android NDK? |
| docs/nix_dev_env.md | ||
|---|---|---|
| 24 ↗ | (On Diff #16081) | What exactly does this mean? |
Comment Actions
Reference prerequisite section for supported workflows
| docs/nix_dev_env.md | ||
|---|---|---|
| 15–18 ↗ | (On Diff #16081) | I originally wrote this before the prerequisite section existed. I can just reference that section now. Probably be a better user experience anyway. Would you like me to just reference the section? ... I went ahead and just referenced it anyway. |
| 22 ↗ | (On Diff #16081) | If I switch Studio and NDK, I think it would sounds odd to me either way reactotron, the Android NDK, or Android Studio. I can add it. |
| 22 ↗ | (On Diff #16081) | Right, but it was never clear in the original documentation what is optional or not. Same with Flipper and idb. I can also just remove the statement and replace it with a link to prerequisite section |
| 22 ↗ | (On Diff #16081) | Removed this comment in favor of just referencing the prerequisite section. |
| 24 ↗ | (On Diff #16081) | It means that it's still in active development for nix support. It's largely documented here: https://github.com/CommE2E/comm/blob/master/docs/dev_services.md. https://www.notion.so/commapp/Nix-Dev-Env-Status-170d0600d2a447fa840eb720fb11e951 will probably give you a better overview of the status; @ashoat didn't want this level of detail in user facing documentation. |
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 ↗ | (On Diff #16084) | |
| 13 ↗ | (On Diff #16084) | We say steps required 'after' Nix installation, but then say workflow 'prerequisites.' While technically both Nix installation and the additional steps need to happen before the workflow is working, it is confusing because they will have to complete the workflow 'prerequisites' after completing the Nix installation steps. To me, prerequisites should happen first, before anything else. If we make the user go through the Nix installation and then ask them to complete "workflow prerequisites" it can be confusing. Maybe we can redefine workflow prerequisites to like "Additional workflow prerequisites" or just reusing "Additional steps" or something else? That way, when the user finishes Nix installation, we can tell them "Here are the additional steps needed to complete workflow etc etc" and the title of that section matches. |
| 15–18 ↗ | (On Diff #16081) | Perfect! This is much better and explicitly links a section that users can find easily. |
| 24 ↗ | (On Diff #16081) | Yeah I agree with @ashoat here. It's more confusing than helpful to the reader. |
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 ↗ | (On Diff #16084) | Don't need the "some" here, it's repetitive and additional is good enough I think, especially since we explicitly link the steps. |
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 ↗ | (On Diff #16084) |
@ashoat specifically requested for this to be first, see https://phab.comm.dev/D4968#144110. This is part of the reason why I moved it to the bottom, as it made more sense for me to:
Then list the status table. But now I have an ordering issue about that status, which needs to mention the end state of certain workflow statuses, before I get into per-workflow requirements. Eventually this status table should be removed, so I don't want to really block other work for getting this transition state polished. |
Comment Actions
Sorry this took so long to review!
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 | Why did we remove the annotations clarifying which workflows need these steps? Is it because pretty much all of the workflows need the steps? My instinct is that it would be better to keep the annotations, so that devs know whether or not they need to read something under the Workflow prerequisites section. Separately, I think we should capitalize "Workflow" in "Workflow prerequisites", as it's a section title. | |
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 |
The additional setup is now documented.
The prerequisites for both nix and the workflows are now closely located together, I don't think it's misleading to say that a workflow is supported if they read the entire prerequisite section.
It shows as "Workflow prerequisites" in my diff... may not have pushed the branch. | |
| docs/nix_dev_env.md | ||
|---|---|---|
| 13 |
Okay, fair.
See proposed edit. | |