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

feat: add support for nested layouts #2

Merged
merged 2 commits into from
Feb 27, 2020
Merged

Conversation

satazor
Copy link
Contributor

@satazor satazor commented Feb 27, 2020

BREAKING CHANGE: the API has greatly changed, please check the README

@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

Merging #2 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #2   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      5    +3     
  Lines          36     68   +32     
  Branches        9     15    +6     
=====================================
+ Hits           36     68   +32
Impacted Files Coverage Δ
src/util/context.js 100% <100%> (ø)
src/LayoutTree.js 100% <100%> (ø)
src/util/use-object-state.js 100% <100%> (ø)
src/util/full-tree.js 100% <100%> (ø)
src/with-layout.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 c55db34...0102a1a. Read the comment docs.

@satazor satazor force-pushed the feat/nested-layouts branch 11 times, most recently from 9456969 to 3ac0e7a Compare February 27, 2020 12:10
Copy link

@zebateira zebateira left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor docs tweaks

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
BREAKING CHANGE: the API has greatly changed, please check the README
@satazor
Copy link
Contributor Author

satazor commented Feb 27, 2020

@zebateira thanks for the review. I'm not sure if I should rename withLayout to withLayoutTree and defaultLayout to defaultLayoutTree. What do you think?

@zebateira
Copy link

I'm not sure if I should rename withLayout to withLayoutTree and defaultLayout to defaultLayoutTree. What do you think?

Hm, I don't have a strong opinion on it, but I think that leaving it as is withLayout is ok since you can have a tree of layouts or not.
Plus, if you then add more nested layouts, it's something that is seamless (simply nest them), so no need to make it so explicit.

@satazor satazor force-pushed the feat/nested-layouts branch 2 times, most recently from 40c749d to 0102a1a Compare February 27, 2020 15:21
@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

Merging #2 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master        #2   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         5    +3     
  Lines           36        68   +32     
  Branches         9        15    +6     
=========================================
+ Hits            36        68   +32     
Impacted Files Coverage Δ
src/LayoutTree.js 100.00% <0.00%> (ø)
src/util/context.js 100.00% <0.00%> (ø)
src/util/use-object-state.js 100.00% <0.00%> (ø)
src/util/full-tree.js 100.00% <0.00%> (ø)

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 c55db34...67136e0. Read the comment docs.

README.md Outdated

Type: `ReactElement` or `function`

In simple cases, you may defined a "static" layout tree, like so:
Copy link

Choose a reason for hiding this comment

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

you have defined => you may have defined

@jgradim jgradim merged commit 1898f83 into master Feb 27, 2020
@jgradim jgradim deleted the feat/nested-layouts branch February 27, 2020 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants