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

Optimize responsive tables #1509

Closed
dev7ch opened this issue Sep 20, 2017 · 10 comments
Closed

Optimize responsive tables #1509

dev7ch opened this issue Sep 20, 2017 · 10 comments
Assignees
Labels
Milestone

Comments

@dev7ch
Copy link
Contributor

dev7ch commented Sep 20, 2017

What steps will reproduce the problem?

In Luya Admin navigate to News > Categories and open e.g. Chrome inspect element toolbar

this happens on a 15" screen (macbook), if page width is less then 1280px

What is the expected result?

image

What do you get instead? (A Screenshot can help us a lot!)

image

This comes from the bs4 css class table-responsive usually its haves an effect under breakpoint sm ( less then 768px )

In compiled luya css we have

@media (max-width: 1280px) {
    .table-responsive {
     ...
    }
}

Why do we have this up to 1280px @luyadev/coredev ? is bootstrap overwritten for this case?

Suggested Solution

We should wrap tables into a div like:

<div class="table-scrollable">
    <table class="table"></table>
</div>

and add a styling like:

.table-scrollable {
    width: 100%;
    overflow-x: scroll;
}

This would give us a much more suitable control to responsive tables.

If it tables would be wrapped we could use the style display: inline-table for table to fix this issue.

The table wrapper could be also applied via js, just instead of add it manually in every html files.

Additional infos

Q A
LUYA Version dev-master
@dev7ch dev7ch changed the title Optimize repsonive tables Optimize responsive tables Sep 20, 2017
@nadar
Copy link
Member

nadar commented Sep 20, 2017

@dev7ch As far as i can see, we should keep the table="table-responsive" and optimize this class instead of adding something new. this way developers does not have to find some misc luya bs4 features somewhere.

  • optimize the table-responsive class
  • do not add new wrapping class (we also would have to add them everywhere!)

@dev7ch
Copy link
Contributor Author

dev7ch commented Sep 20, 2017

the bs4 table-responsive isn´t the best solution for responsive tables, if u search a bit on the web u ll quickly get it.

  1. actually its not more optimizable, we could only reduce the breakpoint, but i guess there is a reaseon why the breakpoint was changed

  2. wrapping could be done via js or jquery, so developers use this feature just on the flow

@nadar
Copy link
Member

nadar commented Sep 20, 2017

  • we will not work with additional wrappers, this should work out of the box according to bs4 usage guide
  • we are using angular, maybe you can watch all tables which habe the class table-responsive and wrap them but currently i am not sure how to watch all tables without creating an directive you have to apply on all tables.

@dev7ch
Copy link
Contributor Author

dev7ch commented Sep 20, 2017

  • ok, but the case i meant is not at Bootstrap tutorial and does not work out of the box (tested on bs4 docs website)

  • sure, agree we have to check properly

explanation:

if we have a table-responsive wich contents are less then 100% width the table isn´t stretched as it should.

This can be only done with a wrapper as a far i can see.

@nadar
Copy link
Member

nadar commented Oct 5, 2017

@TheMaaarc so we should take care of this problem, maybe we can only use this wrapping div for the CRUD table?

@nadar
Copy link
Member

nadar commented Oct 5, 2017

@TheMaaarc Please add this table-responsive wrapper informations to the docks.

TheMaaarc added a commit that referenced this issue Oct 5, 2017
TheMaaarc added a commit that referenced this issue Oct 5, 2017
@TheMaaarc
Copy link
Member

If everything is OK we can close this issue.

@nadar nadar added this to the 1.0.0 milestone Oct 5, 2017
@nadar
Copy link
Member

nadar commented Oct 5, 2017

@dev7ch @TheMaaarc keep the issue open for a while so we can test the changes and see if some unexpected behaviors appear.

@nadar
Copy link
Member

nadar commented Oct 5, 2017

Chromium, Ubuntu: Now i have a scrollbar on each ngrest crud table

screenshot from 2017-10-05 15-59-20

@dev7ch
Copy link
Contributor Author

dev7ch commented Oct 5, 2017

👍

@nadar nadar closed this as completed Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants