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

Start implementing mobile behavior for border-box table #17216

Merged
merged 12 commits into from
Nov 19, 2024

Conversation

oliverguenther
Copy link
Member

Ticket

https://community.openproject.org/work_packages/59248

What are you trying to accomplish?

Trying to have all but the last column in the BorderBoxTableComponent stack on mobile

@oliverguenther oliverguenther force-pushed the feat/mobile-border-box-table branch from 7e51d62 to a3e2a48 Compare November 18, 2024 07:49
Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Looks nice 👍 While clicking through, I found some issues:

  • According to figma: On the meetings page, the project should not display a label in front on mobile
  • On the meetings page, there are different vertical spacings between the elements. That happens on all places where an action button is displayed because the first row takes the height of that button. Thus is it is larger and the space appears bigger.
Bildschirmfoto 2024-11-18 um 14 18 59

And one minor optional comment:

  • Currently, I can enter empty values for the mobile_label and the mobile_columns which does not make much sense I guess. So maybe we should prevent that?

app/components/op_primer/border_box_table_component.sass Outdated Show resolved Hide resolved
lookbook/docs/patterns/12-tables.md.erb Outdated Show resolved Hide resolved
lookbook/docs/patterns/12-tables.md.erb Outdated Show resolved Hide resolved
@oliverguenther
Copy link
Member Author

On the meetings page, there are different vertical spacings between the elements. That happens on all places where an action button is displayed because the first row takes the height of that button. Thus is it is larger and the space appears bigger.

Do you have a suggestion on how to fix this?

@oliverguenther oliverguenther force-pushed the feat/mobile-border-box-table branch 2 times, most recently from 093b666 to c4d1f23 Compare November 18, 2024 16:00
@oliverguenther oliverguenther force-pushed the feat/mobile-border-box-table branch from c4d1f23 to a613bc9 Compare November 18, 2024 16:01
@oliverguenther
Copy link
Member Author

Currently, I can enter empty values for the mobile_label and the mobile_columns which does not make much sense I guess. So maybe we should prevent that?

If you enter empty mobile_columns, it falls back to showing all columns - which I think is a good default

@HDinger
Copy link
Contributor

HDinger commented Nov 19, 2024

On the meetings page, there are different vertical spacings between the elements. That happens on all places where an action button is displayed because the first row takes the height of that button. Thus is it is larger and the space appears bigger.

Do you have a suggestion on how to fix this?

I just pushed a change. The space between the lines is now 4px larger than what is specified in Figma, but imho that is acceptable.

Bildschirmfoto 2024-11-19 um 10 52 44

It looks admitedtly a bit weird, when the text spans multiple lines, but I think it is good enough at least for now. We can then discuss next week with the product team what the best approach would look like.

Bildschirmfoto 2024-11-19 um 11 00 23

@HDinger HDinger self-requested a review November 19, 2024 10:00
@oliverguenther
Copy link
Member Author

Thanks for looking into it. It's definitely better than before 👍

@oliverguenther oliverguenther merged commit 3490947 into dev Nov 19, 2024
11 checks passed
@oliverguenther oliverguenther deleted the feat/mobile-border-box-table branch November 19, 2024 10:34
bsatarnejad pushed a commit that referenced this pull request Nov 20, 2024
* Start implementing mobile behavior for border-box table

* Auto column

* Allow labels

* Hide rows on mobile

* Change order of project and start time

* Add implementation for oidc

* Reimplement wide columns

* Documentation

* Remove custom mobile labels

* Extract heading class

* Fix spacing between rows in mobile BorderBoxTable

* Remove doubled code block

---------

Co-authored-by: Henriette Darge <h.darge@openproject.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants