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 for mobile scrollable tables #49

Merged
merged 10 commits into from
Mar 6, 2021
Merged

Fix for mobile scrollable tables #49

merged 10 commits into from
Mar 6, 2021

Conversation

walterimaican
Copy link
Collaborator

Resolves #31

Root cause analysis revealed that due to conflicting CSS properties on ANTD elements, the table was not rendering properly on mobile devices.

Removing the conflicting CSS yielded a table that spilled out over its parent container. As a result, this pull request includes a hacky solution to dynamically bounding the expenses table within its ancestor elements.

A better long-term solution would involve taking another look at the CSS layout and architecture of the frontend and perhaps overhauling it in the future (see #48).

@jfabellera
Copy link
Owner

Before testing it out on mobile I wanted to make sure everything looked fine as before on desktop, but I found the following issues:

  • on /dashboard
    • Table causes overflow for the entire page (~15px). I would like it stretch to its parent element like before.
    • Table is missing pagination footer.
  • on /expenses
    • Table has no internal overflow i,e, table is as tall as the number of list items it contains, resulting in a large amount of overall page overflow.

And on mobile I found:

  • on /dashboard
    • Table shows expenses but is not scrollable.
    • Table is missing pagination footer.
  • on /expenses
    • Same as desktop problem

Ran this both on my local machine and a development server, and the results were consistent.

@walterimaican
Copy link
Collaborator Author

Line 180 removes the pagination if there is only one page - I added this line while experimenting and forgot to remove it

I'll look further into the issues mentioned

@walterimaican
Copy link
Collaborator Author

walterimaican commented Feb 26, 2021

Desktop

  • dashboard:
    • overflow: Manually adjusted ratio so that there isn't a spillover. Having it stretch to height of parent element is the preferred method, however the existing CSS breaks mobile use. This change is a compromise to see on mobile.
    • pagination footer: Added back (had it set so that if there is only one page, no pagination footer needed)
  • expenses
    • overflow: corrected

Mobile

  • dashboard:
    • scrollable: Table is actually scrollable, it just does not show the scrollbar - limitation with antd table integration
    • pagination footer: fixed, see above
  • expenses:
    • overflow: corrected

@walterimaican walterimaican requested a review from jfabellera March 4, 2021 22:46
@jfabellera jfabellera merged commit 21957f7 into main Mar 6, 2021
@jfabellera jfabellera deleted the scrollable-tables-fix branch March 6, 2021 02:07
@jfabellera
Copy link
Owner

There are some styling issues, but they can be sorted out in another PR and I don't think are a result of this current PR. So I am gonna go ahead and merge this.

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.

Scrollable tables don't show content on mobile
2 participants