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

Put images behind CWs #3979

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@marrus-sh
Contributor

marrus-sh commented Jun 28, 2017

This commit simply puts images inside a post's <StatusContent> so that it will be hidden behind CWs.

@nightpool

This comment has been minimized.

Collaborator

nightpool commented Jun 28, 2017

@akihikodaki why is this marked as a breaking change? I don't think it breaks any technical aspect of the software, and it certainly wouldn't have an impact on upgrades, or federation.

@nightpool nightpool removed the breaking label Jun 28, 2017

@marrus-sh

This comment has been minimized.

Contributor

marrus-sh commented Jun 28, 2017

to be clear this feature is already deployed on dev.glitch.social

<div style={directionStyle} dangerouslySetInnerHTML={content} />
{children}

This comment has been minimized.

@nightpool

nightpool Jun 28, 2017

Collaborator

remove extra newlines

@nightpool

This comment has been minimized.

Collaborator

nightpool commented Jun 28, 2017

I'm -1 on this for a couple of reasons:

  • it provides no indication to the user that there's any sort of media behind the content warning, so it has the potential to be very surprising for users when they open the content warning, expecting only textual content. For example, some people may be okay with textual discussions of specific content, but avoid graphic depictions of the same. If the content warning just mentions the type of content without specifying if it's an image or not, it makes every click of the CW button into a roulette wheel.

  • previous testing in this space (#2666) and my own experience indicates that there are valid usecases that this doesn't provide enough flexibility for. For example, some users use the hidden text to provide extended descriptions or source links for an image, while not cluttering up the main feed. In this case, hiding the image is counterproductive. Another example is using an image as a introduction or a call-to-action for a longer discussion (I've used this when I post mastodon statistics, so people can see the graphs without having to read my rambling analysis)

  • This change leaves the "sensitive" toggle in a no-mans-land of unclear usage, where it doesn't quite "fit in" with the rest of the product in any meaningful way. This is a more minor point, but preferably any pull request that resolved this issue would also take the sensitive toggle into account.

@akihikodaki

This comment has been minimized.

Collaborator

akihikodaki commented Jun 28, 2017

@nightpool I meant it breaks users' assumption which already exists. That could be technical or something different; such distinctions could be done with other tags such as api and deployment.

@akihikodaki

Apart from preference for this change, it has a problem regarding interoperability of other Mastodon instances.
For example, a user of an instance with this change may assume images would be hidden and post without NSFW. However, images are not hidden for users on other instances without this change.
You need to change the federation code as well to avoid that.

@nightpool

This comment has been minimized.

Collaborator

nightpool commented Jun 28, 2017

@akihikodaki While I agree that's an important thing to capture, it's a pretty substantially different thing then what the "Breaking" label has been used for in the past, or what it's trying to convey. If we tagged every pull request with that label that changed users expectations, we would be using it on the majority of pull requests

I don't think the federation code is a blocker here, since there's no real reason this change can't go to all of the mastodon instances, and GNU social instances don't have the concept of sensitive media to worry about anyway. the small period where some instances have upgraded and others haven't isn't that big of a concern in my mind.

@unarist

This comment has been minimized.

Collaborator

unarist commented Jun 28, 2017

Related discussion: #1010

@Cassolotl

This comment has been minimized.

Cassolotl commented Jun 28, 2017

This commit simply puts images inside a post's StatusContent so that it will be hidden behind CWs.

I don't know what this means! Could we see screenshots of that this looks like in action?

@Gargron

This comment has been minimized.

Member

Gargron commented Jun 28, 2017

The StatusContent component is reused in various places where it's not expected to contain more than text. That's one more problem with this PR.

As an alternative, you could extract the hidden/unhidden state upwards from StatusContent to the Status, and have it override state in MediaGallery and VideoPlayer. However, I am not convinced that is how this should work, and aki is right, this change would be breaking people's expectations.

@Cassolotl

This comment has been minimized.

Cassolotl commented Jun 28, 2017

I just tested this on dev.glitch.social (thanks @marrus-sh!), and here's some screenshots. I love it!

Me posting from cybre.space - note the Hide Media button is not selected.
screen shot 2017-06-28 at 14 10 35

How it looks on dev.glitch.social:
screen shot 2017-06-28 at 14 10 56

screen shot 2017-06-28 at 14 11 02

It's great - there's an indication that there's hidden images, it's way tidier than having a big black square showing that doesn't change when you click "show more", stuff like that.

Yes, excellent. I think this could remove the need for the Hide Media button, and be intuitive for new users, and I also think it would be relatively easy for established users to adapt, too.

@Cassolotl Cassolotl referenced this pull request Jun 28, 2017

Open

Combining CW and NSFW #2

@Cassolotl

This comment has been minimized.

Cassolotl commented Jun 28, 2017

@nightpool

it provides no indication to the user that there's any sort of media behind the content warning, so it has the potential to be very surprising for users when they open the content warning, expecting only textual content.

That's not true - there is a little picture image! And four little squares when it's more than one image.

screen_shot_2017-06-28_at_14_10_56

screen shot 2017-06-28 at 14 32 37

This change leaves the "sensitive" toggle in a no-mans-land of unclear usage, where it doesn't quite "fit in" with the rest of the product in any meaningful way. This is a more minor point, but preferably any pull request that resolved this issue would also take the sensitive toggle into account.

I would probably remove the Hide Media button completely, and change the CW button to the eye/crossed-eye symbol. I think that would be very clear for folks speaking all kinds of languages, and less confusing than the current situation.

@Cassolotl

This comment has been minimized.

Cassolotl commented Jun 28, 2017

For example, a user of an instance with this change may assume images would be hidden and post without NSFW. However, images are not hidden for users on other instances without this change.

@akihikodaki How about if posts with CW selected are automatically tagged #NSFW and marked sensitive, so that they would be hidden on other instances?

@unarist

This comment has been minimized.

Collaborator

unarist commented Jun 28, 2017

@Cassolotl

That's not true - there is a little picture image!

Really? I cannot reproduce that behavior on this branch.

Previously we had replaced media urls with an icon like your screenshot, but that feature was removed at 1.4. Also you seems to be applied some user styles, so maybe those customization causes that behavior? (in default style, timestamp should appears on the right of username)

@Cassolotl

This comment has been minimized.

Cassolotl commented Jun 28, 2017

@unarist I don't have any userstyles applied in my browser, so it might be something specific to dev.glitch.social. Would it work for making it sufficiently clear there was an image under the CW? If so, could it be added to this PR?

@nightpool

This comment has been minimized.

Collaborator

nightpool commented Jun 28, 2017

@marrus-sh

This comment has been minimized.

Contributor

marrus-sh commented Jun 29, 2017

@Cassolotl The image icon in the top-right is a feature which was added with collapsed toots and isn't in this PR. (Mastodon puts its timestamp in the top-right and so it wouldn't fit 😉) However, we at GlitchSoc have (as of maybe an hour ago) made a few changes to make toots which contain images more apparent, and I'll hopefully add those along with other improvements to this PR sometime tomorrow. Stay tuned 📻

Many improvements to images in collapsed toots
- Now works on detailed and static pages
- Fixed bug with nested CW / Sensitive Media
- Now apparent which toots contain media
@marrus-sh

This comment has been minimized.

Contributor

marrus-sh commented Jun 30, 2017

Now updated with patches and improvements. It's now obvious when toots have images inside.

image

image

@marrus-sh marrus-sh referenced this pull request Jun 30, 2017

Closed

Images inside of CWs #38

6 of 6 tasks complete
@akihikodaki

This comment has been minimized.

Collaborator

akihikodaki commented Jun 30, 2017

@Cassolotl

How about if posts with CW selected are automatically tagged #NSFW and marked sensitive, so that they would be hidden on other instances?

Sounds good to me. 👍

@marrus-sh

This comment has been minimized.

Contributor

marrus-sh commented Jun 30, 2017

@akihikodaki

A user of an instance with this change may assume images would be hidden and post without NSFW. However, images are not hidden for users on other instances without this change.

This is already something new users assume, and is already the way some frontends (eg., Amaroq) act. The purpose of making this PR in upstream is to make this assumption the standard.

“instances without this change” = instances running old software. I'm not saying that there won't be a transition period, but I think this is a moot point in the long-run.

@Cassolotl

How about if posts with CW selected are automatically tagged #NSFW and marked sensitive, so that they would be hidden on other instances?

I think the decision as to whether or not to unify CWs and sensitive content is beyond the scope of this PR, though.

@akihikodaki

This comment has been minimized.

Collaborator

akihikodaki commented Jul 1, 2017

@marrus-sh

“instances without this change” = instances running old software. I'm not saying that there won't be a transition period, but I think this is a moot point in the long-run.

(disclaimer: the below is just my opinion anyway. the decision to merge this or not is irrelevant.)

Here I clarify my opinion about this problem; the transition period matters even if it is few weeks. Though developers may understand why there are transition periods, it does nothing to do with users. Some users are newcomers and may not know well about Mastodon. Some may be just ignorant about recent changes. If you are kind of those users, how could you be convinced with the situation where you are told to put NSFW for your image while it is hidden for you?
My answer is no. Actually I'm really ignorant for various changes of Web services I use, and I do not think I have to be punished because of that.

@Cassolotl Cassolotl referenced this pull request Jul 4, 2017

Closed

CW should include the image, too #1010

2 of 2 tasks complete
@wxcafe

wxcafe approved these changes Jul 6, 2017

LGTM.
I think this should be merged ASAP too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment