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

Make sidebar items toggleable and save setting in cookie #1903

Merged
merged 1 commit into from
Dec 23, 2022

Conversation

JoeyStk
Copy link
Contributor

@JoeyStk JoeyStk commented Nov 28, 2022

Short description

This PR adds the functionality to make items in the sidebar of content forms expandable and save the setting as a cookie.

Proposed changes

  • Add a class and ids to the items
  • Add functionality to make them toggleable (Side notes: a. only the head of the cards is the trigger, please let me know if you think the entire card should be clickable, b. I didn't add an icon to keep it minimal, please let me know if you prefer an icon, e.g. "+", "-" as commonly used for accordions)
  • Safe value in cookies and retrieve them to remember previous decision

Side effects

  • If you toggle the "Settings on page" section on Desktop screens it still leaves some white space and looks a bit ugly. However my screen is too small to test and/or inspect this issue. If requested I could do this in the next few days on a big screen at the office

Resolved issues

Fixes (partially): #1876


Pull Request Review Guidelines

@JoeyStk JoeyStk requested a review from a team as a code owner November 28, 2022 11:12
@JoeyStk JoeyStk force-pushed the feature/clickable_sidebar_items branch from 510d480 to 8488a7a Compare November 28, 2022 11:14
@codeclimate
Copy link

codeclimate bot commented Nov 28, 2022

Code Climate has analyzed commit 9c70462 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 74.3%.

View more on Code Climate.

@JoeyStk JoeyStk force-pushed the feature/clickable_sidebar_items branch from 8488a7a to 65d381c Compare November 28, 2022 11:23
Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

Thanks, this is very useful, and works great.

My only complaint (besides the two small suggestions below) is that this sets a cookie for every box. Wouldn't it make more sense to set a single cookie containing a serialized JSON object with box names as the key / hidden status as the value?

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 a lot! 🎉

My only complaint (besides the two small suggestions below) is that this sets a cookie for every box. Wouldn't it make more sense to set a single cookie containing a serialized JSON object with box names as the key / hidden status as the value?

Yes, definitely agree! 👍

I would even argue we don't need the value here - just store list of all ids which should be hidden and remove elements from that list which should be shown.

Also, from a security standpoint, it's kind of risky to set cookies to dynamic keys which are derived from the document. I didn't find an immediate way to exploit this, but imagine being able to set the csrftoken cookie to a trivial value just by managing to create a HTML element with the id csrftoken - definitely not ideal...

integreat_cms/cms/templates/pages/page_form.html Outdated Show resolved Hide resolved
@JoeyStk JoeyStk force-pushed the feature/clickable_sidebar_items branch 9 times, most recently from 0d6f32d to ec02774 Compare December 8, 2022 13:56
@JoeyStk
Copy link
Contributor Author

JoeyStk commented Dec 8, 2022

Thank you a lot for your feedback. Originally I wanted to only commit the HTMl files, but accidentally also committed the Typescript files. If you want to you can take a look at the HTML files and check if this okay as it is. And then we can have a chat about how it would be best to set the cookies

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 a lot for your changes! 👍
I really like that we now re-use existing functionality.

And then we can have a chat about how it would be best to set the cookies

I committed a suggestion for the cookie handling - not really perfect yet, but maybe you can play around with it and improve it.
The goal should be to make this also re-usable for other boxes (e.g. on the dashboard) and remember their state as well.

@JoeyStk JoeyStk force-pushed the feature/clickable_sidebar_items branch 2 times, most recently from 80c1917 to 36c50e6 Compare December 9, 2022 10:15
@JoeyStk JoeyStk force-pushed the feature/clickable_sidebar_items branch 4 times, most recently from d7020f8 to d82252f Compare December 9, 2022 17:18
@JoeyStk JoeyStk force-pushed the feature/clickable_sidebar_items branch from d82252f to 40748a3 Compare December 9, 2022 17:38
Co-authored-by: Timo Ludwig <ludwig@integreat-app.de>
@timobrembeck timobrembeck force-pushed the feature/clickable_sidebar_items branch from 40748a3 to 9c70462 Compare December 9, 2022 17:57
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 a lot, really cool feature! 🚀

I noticed that it does not fix #1876 completely (since the issue was about all content forms, also events and POIs), but I think we should merge this PR now since it's a good improvement on its own, and add the event and poi forms later.

I also noticed that it creates problems if the sidebar has only one column (see #1769 (comment)), but I also think it could be part of the separate issue to fix this.

@timobrembeck timobrembeck dismissed charludo’s stale review December 12, 2022 19:53

changes have been addressed

@timobrembeck timobrembeck requested review from a team and removed request for charludo December 12, 2022 19:53
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 👍 Cool feature 😃

@JoeyStk JoeyStk merged commit 42dee0e into develop Dec 23, 2022
@JoeyStk JoeyStk deleted the feature/clickable_sidebar_items branch December 23, 2022 19:48
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