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.

Sign up for GitHub

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

Jump to bottom

feat(Table): add headerRows to allow to render multiple headers #3574

Merged
merged 9 commits into from
May 5, 2019
Merged

feat(Table): add headerRows to allow to render multiple headers #3574

merged 9 commits into from
May 5, 2019

Conversation

abaschen
Copy link
Contributor

Same behavior as renderBodyRow with tableData but for the Headers. Necessary when having more than one line in headers.
If headerRow is set then renderHeaderRow and headerData are ignored.

If accepted, maybe do the same for footer?

@welcome
Copy link

welcome bot commented Apr 18, 2019

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
  • Run yarn test locally to catch errors. This ensures all components still behave as they should.
  • Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
  • Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@codecov-io
Copy link

codecov-io commented Apr 18, 2019 •
edited
Loading

Codecov Report

Merging #3574 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3574      +/-   ##
==========================================
+ Coverage   99.81%   99.81%   +<.01%     
==========================================
  Files         174      174              
  Lines        2730     2734       +4     
==========================================
+ Hits         2725     2729       +4     
  Misses          5        5
Impacted Files Coverage Δ
src/collections/Table/Table.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06b0596...19b96b9. Read the comment docs.

@layershifter
Copy link
Member

layershifter commented Apr 18, 2019 •
edited
Loading

https://codesandbox.io/s/wym08vp8zl?module=/example.js

Can you check this CodeSandbox. Does it satisfiy all of your needs? 🐱

@abaschen
Copy link
Contributor Author

Indeed!! I was currently checking and about to commit tests and changes etc... but with this yes it's perfect!

@abaschen abaschen closed this Apr 18, 2019
@abaschen abaschen deleted the table/multiple-header branch April 18, 2019 13:35
@layershifter
Copy link
Member

I added new docs for shorthand, they will be available on website with next release. But, you can enjoy them right now: https://github.com/Semantic-Org/Semantic-UI-React/blob/master/docs/src/pages/ShorthandProps.mdx

Hope that it will help you 👍

@abaschen
Copy link
Contributor Author

Oops no I've commented too fast. It doesn't satisfy it. how can you have more than one row with rowSpan and so on?

@layershifter
Copy link
Member

Can you please provide example markup in HTML for it?

@layershifter
Copy link
Member

Do you mean something like this?
https://codesandbox.io/s/v0344yp7n7

@abaschen
Copy link
Contributor Author

Yes like this but with shorthands

@abaschen abaschen restored the table/multiple-header branch April 18, 2019 13:54
@abaschen abaschen reopened this Apr 18, 2019
@layershifter layershifter changed the title provide a way to render multiple header like body rows feat(Table): add headerRows to allow to render multiple headers Apr 22, 2019
@layershifter
Copy link
Member

@Techunter please check changes, going to merge after your confirmation 👍

@abaschen
Copy link
Contributor Author

abaschen commented May 3, 2019

Okay for me.

Maybe do the same for footer?

@layershifter
Copy link
Member

layershifter commented May 3, 2019 •
edited
Loading

@Techunter let's do 👍 We can add it in a separate PR

@layershifter layershifter merged commit c9a3418 into Semantic-Org:master May 5, 2019
@welcome
Copy link

welcome bot commented May 5, 2019

Congrats on merging your first pull request! 🎉🎉🎉

robot victory dance

mbakiev pushed a commit to mbakiev/Semantic-UI-React that referenced this pull request Jun 17, 2019
@Techunter @mbakiev
feat(Table): add headerRows to allow to render multiple headers (Se…
2f4370b 
…mantic-Org#3574)

* provide a way to render multiple header like body rows

* Add interface definition

* fix bad copypaste, using tableData instead of headerData. add tests

* rename to headerRows

* cleanup render

* fix UTs
@abaschen abaschen deleted the table/multiple-header branch August 2, 2019 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abaschen @codecov-io @layershifter @Techunter