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

[table] footer row vertical alignment support #1581

Closed
jamessampford opened this issue Jul 16, 2020 · 7 comments
Closed

[table] footer row vertical alignment support #1581

jamessampford opened this issue Jul 16, 2020 · 7 comments
Labels
lang/css Anything involving CSS type/feat Any feature requests or improvements
Milestone

Comments

@jamessampford
Copy link
Contributor

jamessampford commented Jul 16, 2020

Currently, only a table header, row, or cell can adjust its vertical alignment - but this is not supported for a table footer on a table footer row

There may be cases where a cell a row may need multiple lines, but other cells in the footer do not, causing it to look weird

image

At present, the footer will only support middle vertical alignment, whereas all others inherit the style - I suggest allowing vertical alignment to override this default if/when desired

https://fomantic-ui.com/collections/table.html#vertical-alignment

@jamessampford jamessampford added state/awaiting-triage Any issues or pull requests which haven't yet been triaged type/feat Any feature requests or improvements labels Jul 16, 2020
@ko2in ko2in added lang/css Anything involving CSS and removed state/awaiting-triage Any issues or pull requests which haven't yet been triaged labels Jul 16, 2020
@ko2in
Copy link
Member

ko2in commented Jul 16, 2020

@jamessampford I did a quick check and I think there's no need to change for that. You can simple add top aligned variation to your table footer cell. Here's fiddle (https://jsfiddle.net/zvs4qLh8/).

@jamessampford
Copy link
Contributor Author

jamessampford commented Jul 16, 2020

Ah, that one I didn't check, as was trying on the tfoot row - but tfoot row should support it for consistency with thead + tbody

Adjusted fiddle for tfoot row https://jsfiddle.net/9bcsujzo/

@jamessampford jamessampford changed the title [table] tfoot vertical alignment support [table] footer row vertical alignment support Jul 16, 2020
@ko2in
Copy link
Member

ko2in commented Jul 16, 2020

@jamessampford Yes. I'm aware of that. But, by looking the following definition:

.ui.table [class*="top aligned"], .ui.table[class*="top aligned"] {
    vertical-align: top;
}

I assumed that it purposes to align top for all table elements when top aligned is declared. So, attaching top aligned to any table cell should be valid.

I'm not sure if there was a good reason to force middle alignment for the table footer cells as the following specification:

.ui.table>tfoot>tr>td, .ui.table>tfoot>tr>th {
    cursor: auto;
    border-top: 1px solid rgba(34,36,38,.15);
    background: #f9fafb;
    text-align: inherit;
    color: rgba(0,0,0,.87);
    padding: .78571429em .78571429em;
    vertical-align: middle;
    font-style: normal;
    font-weight: 400;
    text-transform: none;
}

If there is no valid reason to force middle alignment for footer cell, then I'll prepare a PR to change this behavior by allowing the inheritance of vertical alignment from the class name.

@jamessampford
Copy link
Contributor Author

jamessampford commented Jul 16, 2020

@ko2in my guess is that it was possibly originally for button alignment

It may well be that top and bottom vertical alignment is added as a separate statement, for example:

.ui.table>tfoot[class*="top aligned"]>tr>td, .ui.table>tfoot[class*="top aligned"]>tr>th,
.ui.table>tfoot>tr[class*="top aligned"]>td, .ui.table>tfoot>tr[class*="top aligned"]>th {
    vertical-align: top;
}

.ui.table>tfoot[class*="bottom aligned"]>tr>td, .ui.table>tfoot[class*="bottom aligned"]>tr>th,
.ui.table>tfoot>tr[class*="bottom aligned"]>td, .ui.table>tfoot>tr[class*="bottom aligned"]>th {
    vertical-align: bottom;
}

this way keeps compatibility with middle as the default

Updated fiddle https://jsfiddle.net/85db9pju/

@lubber-de
Copy link
Member

After some quick testing, i also think we can just change @footerVerticalAlign: middle; to @footerVerticalAlign: inherit;

@ko2in
Copy link
Member

ko2in commented Jul 16, 2020

OK. I'll prepare PR for that.

@ko2in ko2in added the state/has-pr An issue which has a related PR open label Jul 16, 2020
@lubber-de lubber-de added tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build and removed state/has-pr An issue which has a related PR open labels Jul 18, 2020
@lubber-de lubber-de added this to the 2.8.7 milestone Jul 18, 2020
@lubber-de
Copy link
Member

Fixed by #1589

@lubber-de lubber-de removed the tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build label Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS type/feat Any feature requests or improvements
Projects
None yet
Development

No branches or pull requests

3 participants