sindresorhus / refined-twitter

Browser extension that simplifies the Twitter interface and adds useful features
MIT License
1.31k stars 89 forks source link

Show alternative image text below images when available #101

Closed borisschapira closed 6 years ago

borisschapira commented 6 years ago

Fixes #100

ZoeBijl commented 6 years ago

Thank you for this @borisschapira 🙌🏼.

filipekiss commented 6 years ago

It would be ideal if it worked when I open a tweet or open an image to full size. Currently, it only works in the timeline view.

Timeline View Single Tweet Full Size Modal
image image image
jorgegonzalez commented 6 years ago

@filipekiss I'm wondering why it doesn't work when you click on a single tweet, and then works if you hit refresh while still viewing it. Possible bug in onSingleTweetOpen? https://twitter.com/ChromiumDev/status/985944831011639296

image image
filipekiss commented 6 years ago

@jorgegonzalez not sure. The instagram feature works as expected onSingleTweetOpen. Maybe the image alternative function is running too early. But if I open a thread via the notifications tab it works. I'm guessing it's not a onSingleTwitter problem, but I'll take a look later if it's still a problem.

borisschapira commented 6 years ago

Now it works on timeline, single and full size view.

jorgegonzalez commented 6 years ago

Working great now!

I think the div might need a little additional bottom padding for longer alts: https://twitter.com/DavidKPiano/status/988362496154185728

image

I wonder if there's a better way to handle these longer ones.

borisschapira commented 6 years ago

I fixed severa display issues and tried https://twitter.com/heydonworks/media, https://twitter.com/DavidKPiano/status/988362496154185728, and https://twitter.com/MichielBijl/media. Looks ok 😁

borisschapira commented 6 years ago

capture d ecran 2018-04-24 a 12 57 18 capture d ecran 2018-04-24 a 12 57 39 capture d ecran 2018-04-24 a 13 00 01

filipekiss commented 6 years ago

Looks good to me. Just don't forget to fix the CI errors. :shipit: :tada:

To fix the CI errors, run yarn lint (or npm run lint) locally and see what's reported.

jorgegonzalez commented 6 years ago

I think this is ready to be merged.

filipekiss commented 6 years ago

Approved :) Just waiting for @sindresorhus

sindresorhus commented 6 years ago

I think the text should be below the image. That's how it always is and makes it feel more balanced.

sindresorhus commented 6 years ago

Can you mention this feature in the readme feature highlights section?

borisschapira commented 6 years ago

I'm ok with those proposals. Will PR something during the week if nobody has committed s.t.

borisschapira commented 6 years ago

Won't be able to code it for several hours (family time!). Have a nice Sunday :)

sindresorhus commented 6 years ago

No worries and no rush. Enjoy your Sunday :)

borisschapira commented 6 years ago

@sindresorhus I've tried 0.8rem as a font-size, and I'm inconfortable with it. If this is meant to be a a11y/inclusive move, adding text that is smaller than the tweet's text may not be desirable. I would stick with 0.9rem. Colors have been changed to match the light theme, as I don't know how to easily detect light/dark and suppose light is more commonly used. I've tried to append the text instead of prepend. It does not play well when there is 3 or 4 images, Gallery view is broken and, more importantly: reading is interrupted on timeline view, with text above and below the image, which I find rather unpleasant (causes a loss of continuity and meaning). Is that really blocking for you?

borisschapira commented 6 years ago

How it looks like right now:

capture d ecran 2018-04-30 a 12 37 35 capture d ecran 2018-04-30 a 12 37 45 capture d ecran 2018-04-30 a 12 37 58

sindresorhus commented 6 years ago

If this is meant to be a a11y/inclusive move, adding text that is smaller than the tweet's text may not be desirable. I would stick with 0.9rem.

I don't understand. People with disability already have a way to get the alt text with their readers. Your argument for adding this was to remind people to add alt text.

reading is interrupted on timeline view, with text above and below the image, which I find rather unpleasant

I'm of the complete opposite opinion. I don't want my tweet to be interrupted by this fairly useless (for me) text. I understand it's a nice reminder to add alt text, but it's not essential and should be placed last in the tweet, below the image. With how this PR is right now, if I read a tweet, I'll have visually skip the alt text to get to the image.

borisschapira commented 6 years ago

People with disability are very diverse and don't always use readers. Think about colour-blind people, for example. I understand your comment about the feature being useless for you and therefor not-essential. I'll adapt the PR when I'll have a better understanding of how Twitter compute the ratio of images, in order to adapt it to display the alternative text. Some JS computations need to be involved.

borisschapira commented 6 years ago

@sindresorhus How do you feel about this configuration ?

capture d ecran 2018-04-30 a 15 06 27 capture d ecran 2018-04-30 a 15 06 36 capture d ecran 2018-04-30 a 15 06 45

borisschapira commented 6 years ago

/me awkwardly awaits… Is there an issue with what I proposed?

sindresorhus commented 6 years ago

I'm not ignoring this. Was just busy the past weeks. Looks good to me now :)