Skip to content

Redesign: margin for comments#11833

Merged
alecslupu merged 8 commits intodevelopfrom
bug/comments-spacing-top
Nov 13, 2023
Merged

Redesign: margin for comments#11833
alecslupu merged 8 commits intodevelopfrom
bug/comments-spacing-top

Conversation

@Crashillo
Copy link
Copy Markdown
Contributor

@Crashillo Crashillo commented Oct 25, 2023

🎩 What? Why?

Include margin for comments in mobile

📷 Screenshots

Before:
imagen

After:
imagen

♥️ Thank you!

@Crashillo Crashillo added type: fix PRs that implement a fix for a bug project: redesign Barcelona City Council contract labels Oct 25, 2023
@Crashillo Crashillo requested a review from a team October 25, 2023 15:32
github-actions[bot]
github-actions Bot previously approved these changes Oct 25, 2023
github-actions[bot]
github-actions Bot previously approved these changes Oct 26, 2023
@alecslupu alecslupu self-assigned this Oct 26, 2023
@alecslupu alecslupu added this to the 0.28.0 milestone Oct 26, 2023
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR does what is advertised to do, for Blogs engine.

ON meetings and proposals i get this kind of spacing (A gap + Margin top of 2.5rem)
image

On blogs i get a margin-top of 3rem.
image

Can we make sure this is consistent?

@Crashillo
Copy link
Copy Markdown
Contributor Author

Ok, sure.

Just for the record, meeting and proposals (and most of the resources) use the layout_item type, however, blogs do not. Blogs is using the layout-1col (as the most of the forms, but not the resource items). Therefore, it does not apply the same section/spacing stuff, i.e the css behind the scenes is different.

I'll make equal both gaps, good catch! :)

github-actions[bot]
github-actions Bot previously approved these changes Oct 30, 2023
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this is broken in some pages, for instance in the Meeting show page:

Screenshot of the meeting show page with the comments broken

Can you check it out please?

github-actions[bot]
github-actions Bot previously approved these changes Oct 31, 2023
alecslupu
alecslupu approved these changes Nov 1, 2023
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After i provided the approval, i noticed that on meetings while browsing normally, we have the comment tile right below the button.

image

Can you have a look ?

@Crashillo Crashillo requested a review from alecslupu November 2, 2023 11:46
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're pretty near but there are still three to be fixed: Proposals, Posts and (specially) Debates. Can you check them out?

This is what I've reviewed:

OK, they seem consistent between them

Initiative

Initiative comments

Meeting

Meeting comments

Result (Accountability)

Result comments

Project (Budgets)

Budget comments

Not OK (too much gap)

Post (Blogs)

Post comments

Proposal

Proposal comments

Debate

Debate comments


Can you check the last three 🙏🏽 ?

@Crashillo
Copy link
Copy Markdown
Contributor Author

That issue is due to the endorsers_list cell. Notice that the resources you say they're OK, have no Like button. This cell comes with three things:

  • The button Like itself.
  • The small version of the endorsers list (up to 3 elements). It acts like the trigger to the full endorsers list.
  • The full endorsers list.
Screencast.from.07-11-23.10.34.49.webm

Big deal here is the endorsers_list. It is hidden until you click the small version of the list, but it cannot be absolute positioned since it comes: first, horizontally aligned with the tags in desktop but not in mobile; second, it could grow as much as it wants (i.e. many endorsers) overlapping the comments section.

An easy solution (workaround) that comes to my mind now would be to increase the gap a lot. Similar to the current debates page. Doing this, you could add quite a lot endorsers "safely" (but the issue stills). Thoughts?

@andreslucena
Copy link
Copy Markdown
Member

An easy solution (workaround) that comes to my mind now would be to increase the gap a lot. Similar to the current debates page. Doing this, you could add quite a lot endorsers "safely" (but the issue stills). Thoughts?

I'd prefer to have the three of them as the Proposals/Posts page, as Debates is too big the gap.

Also the solution of "let's add more space" doesn't work well, as we don't know how many endorsements could be there. For instance, generating multiple Endorsements I can make this gap big in a Proposal:

More than 200 endorsements in a Proposal (hidden)

As a participant, it doesn't make sense to have this gap.

It only makes sense when we open the endorsers list:

More than 200 endorsements in a Proposal (shown)

You can reproduce it locally with this small snippet:

resource = Decidim::Endorsement.first.resource

Decidim::User.all.each do |author|
  Decidim::Endorsement.create!(resource:, author:)
rescue StandardError
end

And then visit the page of this resource.

As that's not a quick fix, I propose to:

  1. As I said before: to have the Debate page with the same gap as Proposals/Posts, so we can merge this, as it's fixing the issue of consistency between the margin of the different resources
  2. To create a new issue with this new bug ("margin for comments grows when there are lots of likes").

@Crashillo
Copy link
Copy Markdown
Contributor Author

As I said before: to have the Debate page with the same gap as Proposals/Posts, so we can merge this, as it's fixing the issue of consistency between the margin of the different resources

Uff, that's problematic. Debates is OK, as uses the same layout as the rest of the resources. The issue comes when the content is smaller than the aside. Look at the following screenshot:

imagen

Since the layout item is a grid, when the aside block is larger than the main content, it forces this one to stretch. Once the main content includes more text, such gap comes back to "normal".

imagen

So, that big gap will come up always that the main content is smaller than the aside block.

@andreslucena
Copy link
Copy Markdown
Member

Uff, that's problematic. Debates is OK, as uses the same layout as the rest of the resources. The issue comes when the content is smaller than the aside.

Ah Ok, I thought it was something weird happening here but I can see the same behavior in other resources too as you mentioned.

Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There where two pending things that I've mentioned in my last messages:

  1. the Debates show page bug: it's more difficult to solve than what I've initially thought, as @Crashillo mentioned it's actually a "too short" content situation that also happens in other resources. For the moment I think we can ignore it.
  2. the Endorsements situation with the blank space: it was solved with #11991

So, on my side this is ready to be merged. Thanks @Crashillo for your explanations and fixes 👏🏽 👏🏽

@andreslucena andreslucena added the no-backport Pull Requests that should not be backported label Nov 13, 2023
@andreslucena
Copy link
Copy Markdown
Member

Adding the no-backport label as this isn't happening in v0.27

@andreslucena andreslucena dismissed alecslupu’s stale review November 13, 2023 15:00

the meetings margin was solved already

@andreslucena
Copy link
Copy Markdown
Member

As this is assigned to @alecslupu and he already gave a review, he'll have the final merge power on this PR.

Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

LGTM. The Comment spoacing is fixed on desktop and mobile.

Merging with failed CodeCov pipeline

@alecslupu alecslupu merged commit ffbc4de into develop Nov 13, 2023
@alecslupu alecslupu deleted the bug/comments-spacing-top branch November 13, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: blogs no-backport Pull Requests that should not be backported project: redesign Barcelona City Council contract type: fix PRs that implement a fix for a bug

Projects

No open projects
Archived in project
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants