-
Notifications
You must be signed in to change notification settings - Fork 68
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
Improve creating posts/comments #1359
Improve creating posts/comments #1359
Conversation
This is an aside, but we might be able to completely remove the I'm also wondering if we can adjust the positioning of the view source button! From the demo, it feels like it's lumped up with the metadata information which might reduce discovery of the feature. Some ideas:
These are just some initial points. I haven't taken a look at the implementation yet but will do so soon! 😄 |
Good idea! I've removed it from
Sure, I'll play around with it! |
Alright, I added a divider, and also a copy option. Thoughts? qemu-system-x86_64_VS90Yj4YzL.mp4 |
I think that looks better! Thanks for making the change 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a quick review of this and had some points of discussion!
body: post.body ?? '', | ||
isSelectableText: widget.selectable, | ||
), | ||
child: ConditionalParentWidget( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we need to apply the SelectableRegion
here, or would it be more suited as part of the create comment page? (same thing with the comment SelectableRegion)
I guess the main discussion point is whether or not we're expecting to make the post selectable in other areas outside of the create comment page. We already have a way to copy post/comments from #1327!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally put the SelectableRegion
around the whole PostSubview
and CommentContent
widgets, but that had a couple of weird side-effects. For one, I couldn't see the drag handles or the selection highlight color. But the bigger issue is that it allows everything in those widgets to be selectable, including the metadata, which I thought was weird! It seemed to be behave much better when wrapped directly around the CommonMarkdownBody
, and I don't think it causes any harm, even if this is the only place we use it. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh okay, you make a good point about allowing metadata to be selected! We'll keep it as it!
This should be ready to go, depending on your thoughts on the unresolved comment! |
Pull Request Description
This PR includes a few changes related to creating posts/comments.
MarkdownWidget
'sselectable
flag doesn't support selecting across multiple paragraphs, so I wrapped it in aSelectableRegion
which is more flexible.Screenshots / Recordings
qemu-system-x86_64_2nQKIqtlHs.mp4