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

Port the “unfold thread” feature from Mastodon's UI to glitch-soc flavour #401

Conversation

ClearlyClaire
Copy link

@ClearlyClaire ClearlyClaire commented Mar 28, 2018

This is a very much in progress PR aiming to implement he “unfold thread” feature of Mastodon (fixes #387). Unfortunately, as the code has diverged quite a bit and is pretty hard to reason about, it will take some work.

  • Port related changes to column headers
  • Refactor the collapsed/extended logic to make it easier to reason about
  • Port the feature itself

@ClearlyClaire ClearlyClaire force-pushed the glitch-soc/features/unfold-thread branch 4 times, most recently from f5aae55 to ad830db Compare March 28, 2018 19:13
@ClearlyClaire
Copy link
Author

Porting this feature really is a pain, in terms of user interface, because of collapsed toots. The crux of the problem is the following:

  1. This change makes the folded/unfolded status for content warnings shared across every view of a toot
  2. So far, the collapsed/not-collapsed state of a toot is specific to a view of a toot. It makes sense because one may want toots in their notifications column collapsed while not having them collapsed in their TL
  3. It does not make much sense for a collapsed toot to have have its content warning unfolded, especially as that would cause the “fold/unfold” button to not have seemingly no effect on collapsed toots

In the current state of this PR, point 3 is ignored, and the “CW unfolded but collapsed view” state is possible, which is a regression.
I'm not too sure how to proceed, but I'm considering changing 1 to keep the previous behavior, but it will be more work and it will not bring Mastodon and glitch-soc codebases closer…

@hannahwhy hannahwhy requested a review from marrus-sh March 29, 2018 09:19
@ClearlyClaire ClearlyClaire force-pushed the glitch-soc/features/unfold-thread branch 4 times, most recently from fed2aa6 to adf1274 Compare April 2, 2018 11:23
@marrus-sh
Copy link
Member

marrus-sh commented Apr 3, 2018

Yeah I am not a fan of having CW folding status shared across every instance of the toot for a number of reasons:

  1. In notifications it makes things really awful if you have a bunch of notifications on the same, CW'd toot, and then you expand it (all of the instances of the toot in the notifications bar expand, causing you to completely lose your place)
  2. In general, I think rendering details should be kept out of redux (this is a personal stance which upstream does not share) and that the redux store should be reserved just for API stuff

I think upstream's approach to this was kinda hacky and not great from a useability perspective and results (imho) from an overreliance on Redux as a catch-all solution to every problem, which it isn't :P. Instead this should be handled via a passed prop/state from the conversation column (React idiomatic if a little complicated) or via Events (the better, cleaner solution, but not in line with any of the existing codebase).

@ClearlyClaire
Copy link
Author

Alright, this is something I wasn't too sure about. I'll try to rework the PR to fold/unfold the toots in the thread view only, not using the Redux store for that.

@ClearlyClaire ClearlyClaire force-pushed the glitch-soc/features/unfold-thread branch 2 times, most recently from dbf3803 to 47bc24c Compare April 4, 2018 10:21
@ClearlyClaire ClearlyClaire changed the title [WiP] Port the “unfold thread” feature from Mastodon's UI to glitch-soc flavour Port the “unfold thread” feature from Mastodon's UI to glitch-soc flavour Apr 4, 2018
@ClearlyClaire ClearlyClaire force-pushed the glitch-soc/features/unfold-thread branch from 8be910b to 967f95e Compare April 4, 2018 13:02
@ClearlyClaire
Copy link
Author

Updated to not use redux. The CW fold/unfold state is now specific to each view of the toot, unlike in tootsuite/mastodon. Tried to keep the differences with tootsuite/mastodon minimal nevertheless.

@hannahwhy
Copy link
Member

@marrus-sh When you get a chance, can you look over these changes?

@ClearlyClaire ClearlyClaire force-pushed the glitch-soc/features/unfold-thread branch from 967f95e to 23c61b6 Compare April 19, 2018 09:29
@ClearlyClaire ClearlyClaire force-pushed the glitch-soc/features/unfold-thread branch from 23c61b6 to b383c06 Compare April 22, 2018 17:20
@beatrix-bitrot beatrix-bitrot merged commit f4ed382 into glitch-soc:master Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port “Add show more/less toggle for entire threads in web UI”
4 participants