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

RFC 50: Commenting in the Wagtail Editor #50

Merged
merged 4 commits into from
Feb 1, 2022

Conversation

jacobtoppm
Copy link
Member

@jacobtoppm jacobtoppm commented May 14, 2020

@jacobtoppm jacobtoppm changed the title WIP: First draft of commenting RFC Commenting in the Wagtail Editor May 14, 2020
@jacobtoppm jacobtoppm changed the title Commenting in the Wagtail Editor RFC 50: Commenting in the Wagtail Editor May 14, 2020

### Field and Block Commenting

This will be the standard form of commenting. Comments can be left on either a field or a StreamField block.
Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure I understand the value of adding a comment on a StreamField block, as opposed to just the field(s) within a block.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was something that came out of preliminary UX discussion - they thought it might be too fine grained/end up with button overload to allow commenting on every field inside every block, so suggested limiting it to blocks instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like we're mixing terminology here - technically everything nested at any level inside a StreamField is a block (including things like CharBlock). Is the proposal to only allow comments on immediate child blocks of the top-level StreamBlock of a StreamField, then?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure we discussed having both, and the content path example on line 34 suggests that it would be possible to have comments on fields within a structblock.

One limitation of content paths is they cannot reference things in listblocks because the items in those don't have IDs that persist across revisions. It's a similar issue to recording offsets in rich text fields so we will probably have to update the content path on edit, or limit this to only allow comments on listblocks as a whole and not children within them.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I think things should ideally be commentable at any level - I believe the UX concern was largely about individual blocks within StructBlocks.

Copy link
Member

Choose a reason for hiding this comment

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

If there’s already been UX considerations / wireframes for this could you share them in the RFC @jacobtoppm?

GitHub allows commenting on any line of code, and word processor like Google Docs / Dropbox Paper allow commenting on any character of any line. So I don’t really see the concern with comments being too fine grained, or the risk of button overload (assuming the UI is designed similarly to other commenting systems).

I’m not quite sure this needs addressing as part of the RFC though.

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'm afraid there's not been very much yet - what I've said about the UX has been the product of a half hour conversation with them, that's all.

To clarify: are you suggesting we should allow inline commenting on any field? Or just that we should allow commenting on any block/field combo?

For the former, I'm suggesting allowing only inline commenting in Draftail because of the technical aspect - we can track how an inline entity moves and changes much more easily within a rich text field. UX suggested not enabling this by default just because it's a somewhat different user experience from commenting on a field as a whole, and they thought it might be confusing for the user.

text/050-commenting.md Show resolved Hide resolved
@PresidentNick
Copy link

Nice, I was looking for something like this yesterday. I wasn't aware of wagtail-review, I will try it out. I also think it is a great idea to have it built into the editor fields inside wagtail as laid out in your abstract! What is the usual timeline and steps for RFCs on wagtail? I've never followed an RFC from inception to production.

@jacobtoppm
Copy link
Member Author

Nice, I was looking for something like this yesterday. I wasn't aware of wagtail-review, I will try it out. I also think it is a great idea to have it built into the editor fields inside wagtail as laid out in your abstract! What is the usual timeline and steps for RFCs on wagtail? I've never followed an RFC from inception to production.

Thanks for your interest! You can see a summary of the process on the readme, here: https://github.com/wagtail/rfcs. The next big step for this one would be discussing it again in a Wagtail core team meeting, deciding whether or not to accept it - then actual development. However, the timeline can definitely vary substantially, so it's hard to give an estimate here, though someone else might have a better idea than me - some have lingered for a long time, while some (like Wagtail Transfer) have been implemented within the next couple of months. As we have development time for this, assuming we accept some variant of the API changes, I'd hope more on the months scale for this one, aiming to go in with or shortly after the new workflow feature.

@thibaudcolas
Copy link
Member

@jacobtoppm since we now have commenting in Wagtail, I presume we can merge this RFC?

@jacobtoppm
Copy link
Member Author

Yep! Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants