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

Remove gap between sidebar boxes (for POIs) #2043

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

JoeyStk
Copy link
Contributor

@JoeyStk JoeyStk commented Jan 31, 2023

Short description

With introducing the collapsible boxes to the page, event and POI form we created a gap between the boxes when certain boxes are collapsed. This is mainly due to two factors: a. using grid instead of flex, b. the two sidebars for medium and 4xl screens aren't wrapped inside one parent div. These changes are going to be relevant for all post types, however in this PR (at least up until now) I focused on POIs only. If we fixed everything for POI I will extend these changes to events and pages one by one.

For a graphical summary on what I tried to fix see the screenshots here:
Before:
grafik

After:
grafik

Proposed changes

  • Using grid only for 3xl screens
  • Add parent div for two sidebars (for medium and 4xl screens)
  • Remove flex-auto for the editor so it doesn't create blank space
  • Adjust spacing between the boxes

Side effects

While testing this, I didn't run into any. However I can't imagine that there are now, since this is a major change. So I would ask you to test this PR thoroughly 😊
Please also check if you can find classes, that aren't used anymore, so I can remove them :)

Resolved issues

Fixes: Parts of #2032


Pull Request Review Guidelines

@JoeyStk JoeyStk requested a review from a team as a code owner January 31, 2023 10:49
@codeclimate
Copy link

codeclimate bot commented Jan 31, 2023

Code Climate has analyzed commit 2b2ba3a and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 75.0% (0.0% change).

View more on Code Climate.

@JoeyStk JoeyStk force-pushed the bugfix/remove_gap_between_collapsed_boxes branch from 987d6e5 to 3f432d2 Compare January 31, 2023 10:50
Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

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

Thank you for PR 👍 Looks very nice 😃

Do we need an entry in changelog?

Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

No gap now ✅
But there is a regression change:
If I collapse some blocks and re-open form, blocks are reordered randomly.
image
It's not reproduced on develop branch - all blocks always retain their position.
Do we want and can avoid it?

UPD. it should probably be a "comment", not a "change request". But I don't know if I can change it already 🙈

@JoeyStk JoeyStk force-pushed the bugfix/remove_gap_between_collapsed_boxes branch 3 times, most recently from 566640f to f17cf8e Compare February 6, 2023 16:47
@JoeyStk
Copy link
Contributor Author

JoeyStk commented Feb 6, 2023

Thank you for your reviews!

@MizukiTemma I will add an entry to the changelog once I have extended this to events and pages :)
@selulanova This is expected behaviour, that was part of this issue. However since I created the PR we have added the option to switch off the automatic distribution of the boxes. I have rebased my branch now, which means the boxes shouldn't be automatically reorganized by default, but you could enable it in the user settings. As soon as the test run through you can test it again :)

Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

Thank you 👌
One more regression issue I have noticed:
If I minimize the window, boxes go down and gap between left and right columns is missed
image

@JoeyStk JoeyStk force-pushed the bugfix/remove_gap_between_collapsed_boxes branch 2 times, most recently from 4a60374 to b63a63c Compare February 8, 2023 14:55
@JoeyStk
Copy link
Contributor Author

JoeyStk commented Feb 8, 2023

Oh, good catch. I don't know why I didn't see this. The gap-2 is of course not working anymore. I should have thought of that. I think it's fixed now :)

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Hmm, somehow I noticed problems on all tested screen resolutions...
In my opinion, the spacing between the boxes and the columns should be consistent. I think, 4rem looks better than 2rem, but the most important thing is that it's consistent.

1700px and below

Screenshot 2023-02-10 at 13-16-16 Integreat Editorial System

1700px - 2100px

Screenshot 2023-02-10 at 13-16-40 Integreat Editorial System

2100px and above

Screenshot 2023-02-10 at 13-15-32 Integreat Editorial System

@JoeyStk JoeyStk force-pushed the bugfix/remove_gap_between_collapsed_boxes branch from b63a63c to a751d65 Compare February 14, 2023 14:44
@JoeyStk
Copy link
Contributor Author

JoeyStk commented Feb 14, 2023

Very good catch. Thank you for the detailed feedback. In some parts this goes beyond what I wanted to do with this PR and in other parts I created this issue during a rebase. I adjusted the spacing for all boxes and gaps to class category 4 (e.g. gap-4 or mb-4, =1rem). I was only able to double check if my changes work for screens smaller than 2100px. For screens above 2100px I can only test it earliest next week

Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

Now I cannot find anymore problems, looks good!
I would just reword the changelog entry.

CHANGELOG.md Outdated Show resolved Hide resolved
@JoeyStk JoeyStk force-pushed the bugfix/remove_gap_between_collapsed_boxes branch from 584a748 to 2b2ba3a Compare February 17, 2023 17:21
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.

None yet

4 participants