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

Fix/improve community post list styling #3985

Draft
wants to merge 14 commits into
base: development
Choose a base branch
from

Conversation

kommunarr
Copy link
Collaborator

Improve community post styling & fix list styling

Pull Request Type

  • Bugfix

Related issue

closes #3984

Description

Fixes community post styling on the list view (as in, is legible and actually pretty). Wraps each community post in its own ft-card for list and grid views, and equalizes the list and grid views (given that there does not seem to be a good way to actually display community posts in a grid).

Screenshots

Screenshot_20230902_021401
localhost_9080_(iPad Air)
localhost_9080_(iPhone 12 Pro)

Testing

1, Under General Settings, set Video View Type to "List"
2. Navigate to this channel
3. Navigate to the Community tab
4. Ensure that view appears correctly
5. Check with smaller device size (Chrome devtool)
6. Repeat for Grid view
7. Check with smaller device size (Chrome devtool)

Desktop

  • OS: OpenSUSE
  • OS Version: Tumbleweed
  • FreeTube version: 0.19.0

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 2, 2023 07:28
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 2, 2023
@absidue
Copy link
Member

absidue commented Sep 2, 2023

Community posts were only ever meant to be displayed in a list view, but I'm guessing it was only ever tested with global grid, forced listed, not global list + forced list.

That's why he have the display list option.

@absidue
Copy link
Member

absidue commented Sep 2, 2023

I guess the question is, instead of letting the the ft-community-post component even accept a grid and list value, maybe just doing it's own thing always in list mode would require less workarounds? (can this be fixed in the ft-community-post component without requiring "global" overrides?)

Especially requiring the community-posts list on the channel page to be almost at the root, is going to make it impossible to create a shared tab component, because it can't be in a normal nested layout anymore.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Sep 2, 2023

I guess the question is, instead of letting the the ft-community-post component even accept a grid and list value, maybe just doing it's own thing always in list mode would require less workarounds?

Yeah I was of two minds about this - it technically isn't doing any good right now, but I thought I'd keep it this way in case someone does have a good way of differentiating between the two styles (+ didn't know if overriding the user's list/grid preference would be a bad idea to start doing). The only difference is that a handful of CSS properties have been added to the grid view version to equalize it. But I don't really care much either way, so I can remove that if we have any reason to at all.

(can this be fixed in the ft-community-post component without requiring "global" overrides?) Especially requiring the community-posts list on the channel page to be almost at the root, is going to make it impossible to create a shared tab component, because it can't be in a normal nested layout anymore.

Is this in reference to the putting it outside of the ft-card object covering a lot of the tabs in Channel.vue? Yeah, this one was giving me a lot of decision grief - I think it's nice to have a way to allow these ft-element-lists to be wrapped in ft-cards, but taking it out of there is messier for sure. How about this: I put it back in there, and add styling to ft-card.css that overrides the parent styling if it has any ft-card children (with a certain class) (not sure if :has has been implemented yet at our Electron version, but I'll check)?

@absidue
Copy link
Member

absidue commented Sep 2, 2023

We already override the preference in a few places, e.g. the playlists page. The community posts were always meant to override the users global choice (that's why the display="list" prop exists), it was just only made for when the global choice is grid, which is why it worked fine for that but not list.

I partially fixed it a while back, by making the community posts think that the global choice was always grid, but never got round to fully fixing it.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Sep 2, 2023

We already override the preference in a few places, e.g. the playlists page. The community posts were always meant to override the users global choice (that's why the display="list" prop exists), it was just only made for when the global choice is grid, which is why it worked fine for that but not list.

I partially fixed it a while back, by making the community posts think that the global choice was always grid, but never got round to fully fixing it.

Ah okay, I was thinking that to some extent when I saw it at the top level, but then I saw we were still grabbing their preference on the ft-community-post page and was confused. I can just remove it from the community posts directly then as you've said. Thanks!

@absidue
Copy link
Member

absidue commented Sep 2, 2023

Yeah it's a bit weird, the list has to be in list layout but the post itself, has to be in grid layout for it to work well.

@absidue
Copy link
Member

absidue commented Sep 3, 2023

I was able to get the community posts to look identical in both the list and grid global layout, with these simple changes. Maybe I'm misunderstanding what this pull request is supposed to achieve, but it seems like it might be overcomplicating this rather simple change (modifying the way ft-card works and using component is, seems like the wrong approach to me).

Edit: Discussed on Matrix, the original comment made incorrect assumptions, there is more to the pull request than just this, but we managed to discuss a potential way of avoiding so many changes

diff --git a/src/renderer/components/ft-community-post/ft-community-post.js b/src/renderer/components/ft-community-post/ft-community-post.js
index a7086c29..c7d1c7f7 100644
--- a/src/renderer/components/ft-community-post/ft-community-post.js
+++ b/src/renderer/components/ft-community-post/ft-community-post.js
@@ -52,10 +52,6 @@ export default defineComponent({
         slideBy: 'page',
         navPosition: 'bottom'
       }
-    },
-
-    listType: function () {
-      return this.$store.getters.getListType
     }
   },
   created: function () {
diff --git a/src/renderer/components/ft-community-post/ft-community-post.vue b/src/renderer/components/ft-community-post/ft-community-post.vue
index 870a6680..d528af50 100644
--- a/src/renderer/components/ft-community-post/ft-community-post.vue
+++ b/src/renderer/components/ft-community-post/ft-community-post.vue
@@ -1,9 +1,8 @@
 <template>
   <div
     v-if="!isLoading"
-    class="ft-list-post ft-list-item outside"
+    class="ft-list-post ft-list-item outside grid"
     :appearance="appearance"
-    :class="{ list: listType === 'list', grid: listType === 'grid' }"
   >
     <div
       class="author-div"

@kommunarr
Copy link
Collaborator Author

So it's doing a bit more than that if you see the picture and description; but I will remove the complicating stuff.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2023

Conflicts have been resolved. A maintainer will review the pull request shortly.

@kommunarr kommunarr force-pushed the fix-community-post-list-styling branch from 74a4345 to 633cdec Compare October 8, 2023 14:05
Copy link
Contributor

github-actions bot commented Dec 8, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@efb4f5ff-1298-471a-8973-3d47447115dc

Is there anything blocking this except for the conflicts?

Copy link
Contributor

github-actions bot commented Jan 5, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@kommunarr kommunarr force-pushed the fix-community-post-list-styling branch from d17db07 to ae27760 Compare January 5, 2024 03:40
@kommunarr
Copy link
Collaborator Author

Community posts are still broken, I think, so that needs to be handled

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@kommunarr
Copy link
Collaborator Author

This one is a nice lil visual improvement for all users, and it does fix Community Post styling for people who use List view, but honestly, I haven't heard a single soul complain about this, meaning very few people probably actually use List view (and/or read community posts on FreeTube). Too low priority for me to care about fixing compared to other items on my list.

@kommunarr kommunarr added the PR: low priority For pull requests that don't require a fast review. e.g. code cleanup label Apr 20, 2024
@kommunarr kommunarr marked this pull request as draft April 20, 2024 14:32
auto-merge was automatically disabled April 20, 2024 14:32

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: changes requested PR: low priority For pull requests that don't require a fast review. e.g. code cleanup PR: merge conflicts / rebase needed PR: WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Community page broken with list view enabled
3 participants