-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add separator between columns #18492
base: master
Are you sure you want to change the base?
Add separator between columns #18492
Conversation
aae8ba3
to
89067d0
Compare
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #18492 +/- ##
============================================
- Coverage 56.43% 56.37% -0.06%
- Complexity 16074 16096 +22
============================================
Files 635 637 +2
Lines 63450 63537 +87
============================================
+ Hits 35811 35822 +11
- Misses 27639 27715 +76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
04ada4a
to
7dce338
Compare
Is it confusing for RTL languages to have numbers aligned to the end of the cell? |
@MauricioFauth The problem is there is no line that separates the columns so the content of a column appears like if it belongs to the next one. Since there is no vertical separator between columns I think when the content is well aligned with the header this makes it obvious which column it belongs. 🤔 |
So the issue is that there's no clear separation between cells, and not the alignment itself. |
Yeah sure! I don't have a problem with the numbers in the end of cell, but we need some sort of separation between columns so we don't have that confusion between columns. Should I update it with columns separators, I think this will solve the issue. What do you think? |
Maybe a bit more padding or add a subtle border for the pmahomme theme. But I think this should be done in the |
This pull requests contains too many commits, in most cases it is caused by wrong merge target. In case you have forked |
e81ae14
to
22f0ba2
Compare
Signed-off-by: faissaloux <fwahabali@gmail.com>
Fixes phpmyadmin#18471 Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev>
Signed-off-by: faissaloux <fwahabali@gmail.com>
Signed-off-by: faissaloux <fwahabali@gmail.com>
Signed-off-by: faissaloux <fwahabali@gmail.com>
Signed-off-by: faissaloux <fwahabali@gmail.com>
Signed-off-by: faissaloux <fwahabali@gmail.com>
Signed-off-by: faissaloux <fwahabali@gmail.com>
Signed-off-by: faissaloux <fwahabali@gmail.com>
5f4a07a
to
58eb79b
Compare
Signed-off-by: faissaloux <fwahabali@gmail.com>
334be70
to
89773db
Compare
@MauricioFauth I have added separator between column and ready for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there no bootstrap way of doing this?
Signed-off-by: faissaloux <fwahabali@gmail.com>
5a8a3a6
to
e9be10a
Compare
tbody { | ||
th, | ||
td { | ||
border-right: 1px solid #00000017; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change still needed on a bordered table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
@@ -292,7 +292,7 @@ | |||
{% endif %} | |||
|
|||
<div class="table-responsive-md"> | |||
<table class="table table-striped table-hover table-sm table_results data ajax w-auto" data-uniqueId="{{ unique_id }}"> | |||
<table class="table table-striped table-bordered table-hover table-sm table_results data ajax w-auto" data-uniqueId="{{ unique_id }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced that this should be applied to all themes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@williamdes what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty vote, I am not the themes guru here 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sooo! should I revert the last change concerning the table-bordered
class 🤔
In this PR I have added columns separation which was a bit confusing especially on numbers data.