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

feat(templates): Profile tables are now responsive with accordion in smaller screens #4453

Merged
merged 11 commits into from
Nov 5, 2024

Conversation

elisa-a-v
Copy link
Contributor

@elisa-a-v elisa-a-v commented Sep 13, 2024

This PR makes profile tables responsive by adding an accordion feature that hides columns in smaller screens. To do this a script and some CSS was implemented that allows to easily make any table responsive.

To make tables responsive, include a script with src="{% static "js/responsive-table.js" %}", add the class .responsive-table-wrapper to the <table> element, and add an attribute data-label to each <th> element to be labelled in the mobile version with stacked columns.

Docket alerts:

docker alerts

Docket notes:

docket notes

Opinion notes:

opinion notes

@CLAassistant
Copy link

CLAassistant commented Sep 13, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ elisa-a-v
❌ elisa-av
You have signed the CLA already but the status is still pending? Let us recheck it.

@elisa-a-v elisa-a-v changed the title WIP: feat(templates): Notes tables are now responsive with accordion in smaller screens feat(templates): Profile tables are now responsive with accordion in smaller screens Sep 17, 2024
@elisa-a-v
Copy link
Contributor Author

@mlissner I'd say this is almost ready, but I just noticed that the notes tables have a button in the last header to search notes, which isn't shown in the mobile version. I guess I could add it somewhere else and display it in mobile only, like above the table? or I could move the button entirely. let me know what you think

image

@mlissner
Copy link
Member

let me know what you think

I think it's fine that the search isn't there in mobile. Thus is the way with mobile....

Copy link
Contributor

@ERosendo ERosendo left a comment

Choose a reason for hiding this comment

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

@elisa-a-v The code looks great! I have a few suggestions to improve the responsive design:

  1. I think we could enhance the animation of the up/down arrow. Currently, it seems to rotate around one of its vertices. A more common approach is to rotate the arrow around its center point. Here's a GIF demonstrating the desired behavior:

    • Current implementation:

    Screen Recording 2024-10-31 at 12 56 40 PM

    • Desired behaviour

    Screen Recording 2024-10-31 at 12 54 38 PM

  2. I noticed that the fa-list icon is a bit too close to the text in the accordion header. I think adding a non-breaking space between the icon and the text would improve the visual spacing. Here's a screenshot:

    Screenshot 2024-10-31 at 1 03 43 PM

  3. While reviewing the notes table, I noticed that the modal window for updating a note isn't centered correctly on smaller screens. Here's a screenshot:

    image

    While this issue might be out of scope for this specific PR, I'm flagging it here as it's related to our overall responsive design efforts.

@mlissner
Copy link
Member

Thanks Eduardo. It's probably worth fixing #2, but I don't think it's worth bothering on 1 or 3, since we'll be revamping the details and switching to tailwind fairly soon. A little funkiness temporarily is fine, I think.

@elisa-a-v
Copy link
Contributor Author

@ERosendo I only fixed # 2 as @mlissner suggested, should I open issues for the other 2 points?

@mlissner
Copy link
Member

mlissner commented Nov 5, 2024

Normally I'd say yes, Elisa, I love filing bugs, but I think these will just get fixed no matter what in our design overhaul.

Eduardo, merge if you're ready!

Thank you both!

@ERosendo ERosendo merged commit 417a674 into freelawproject:main Nov 5, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants