readthedocs.org
readthedocs.org copied to clipboard
readthedocs
GitHub App: post comments on PRs
Ref https://github.com/readthedocs/readthedocs.org/issues/11780
This looks like a good start, we should port the logic from the GH action over, and if possible try to get files changed working here as well!
Should the comment also respect the ignore patterns from the addon config? It's also kind of confusing that we have these settings in two places... in addons and in the PR form.
Yes we should respect the ignore patterns from addon config.
What's the "PR form" you refer to? I only see this config in the addons form https://app.readthedocs.org/dashboard/docs/addons/edit/ under "File Tree Diff"
I'm :+1: on moving forward with a minimal example. I don't want to get stuck on nitpicking the text, but think we can get it even more minimal.
What's the "PR form" you refer to? I only see this config in the addons form https://app.readthedocs.org/dashboard/docs/addons/edit/ under "File Tree Diff"
@humitos we have the PR form at https://app.readthedocs.org/dashboard/docs/pull-requests/, that's where this new setting will be listed. I think we may need to move the ignored files settings into there, so the addon setting is only for showing or hiding the addon.
I was hoping to start more minimal here too. We're getting caught up a bit here because this is adding a lot of features we didn't have prior and we didn't plan what this was going to look like.
I wonder about just hiding the full Files changed listing under details, if we want to go fully minimal to start.
I'd prefer this too, it would function like the visual diff dropdown in the docs -- you have to click that to see the files. I think this is perfectly fine. The first, duplicate list of files outside the details feels at odds with the details view and this is confusing.
Also, we're using emoji before explaining their meaning. The modified/added/deleted summary does a great job of explaining these though.
So I think the files table in the details view also wins on clarity too. This line should be visible if we're showing a table of changes.
I propose to simplify what we have here based on the comments and feedback, so we move forward with a minimal version that we can tune as we go. Summarizing, we should:
- remove the ↩️ arrows to go back to the original file
- remove the bullets listing the top 5 changed files
- move the whole "Files changed" section into the
<details>so it's hidden by default - show the summary of files added/modified/deleted outside the
<details>so is visible by default
I propose to simplify what we have here based on the comments and feedback, so we move forward with a minimal version that we can tune as we go. Summarizing, we should:
remove the ↩️ arrows to go back to the original file
remove the bullets listing the top 5 changed files
move the whole "Files changed" section into the
<details>so it's hidden by defaultshow the summary of files added/modified/deleted outside the
<details>so is visible by default
This sounds like a great place to start. I agree there's a bunch more features we can add, but I think we should keep it simple so we can get it out the door. It's already a lot better than what we have with the GH action, which is the main thing I was looking to recreate in terms of functionality.
I think we will definitely tweak the format of this message over time, but I just want to get something done, and stop debating small UX decisions without having anything shipped.
Are we okay with this?...
That looks good. The emoji in the status modified/added/deleted line did help, I think that is going to be something we end up wanting back -- breaking out the modified/added/deleted line from the details and the details dropdown becomes just "Show all file changes".