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

Evil159/footer support rebased #31

Merged
merged 5 commits into from
Apr 9, 2019
Merged

Conversation

bryankeller
Copy link
Contributor

@bryankeller bryankeller commented Apr 4, 2019

Details

This PR is a rebased version of #23. I went ahead and incorporated all of the feedback that I would have asked you to implement @evil159 in the last commit 433d0bc

Some notable changes I made:

  • Simplified the footer layout logic in SectionModel. I want to avoid introducing additional flags and state. The addition of a new return-early case in the core layout function also made me nervous. The new code is less efficient, but more straightforward. I plan to refactor this function anyways so that recompute / invalidation is more explicit.
  • Fixed tests (the original footer layout logic had a bug, and so the tests were buggy as well. See bf47285#diff-187bbc570783ead7c85afd619305a795R562 - how could a footer have 0 width?)
  • Separated MagazineLayoutSupplementaryViewVisibilityMode into a MagazineLayoutHeaderVisibilityMode and MagazineLayoutFooterVisibilityMode. The reasoning for this is to ensure that this does not introduce a breaking API change.
  • Minor indentation updates
  • Minor naming updates
  • Minor file organization updates

Related Issue

#18

Motivation and Context

How Has This Been Tested

Ran automated tests.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@bryankeller bryankeller added the enhancement New feature or request label Apr 4, 2019
@bryankeller bryankeller mentioned this pull request Apr 4, 2019
9 tasks
@bryankeller bryankeller force-pushed the evil159/footer-support-rebased branch 2 times, most recently from f2044a3 to 414e5c4 Compare April 4, 2019 21:10
@bryankeller bryankeller force-pushed the evil159/footer-support-rebased branch from 414e5c4 to 73ac07c Compare April 4, 2019 22:01
Copy link

@evil159 evil159 left a comment

Choose a reason for hiding this comment

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

Thank you so much for going through the trouble of fixing all the mishaps.

• Simplified the footer layout logic in SectionModel

Yes, it makes perfect sense to have it like that.

• how could a footer have 0 width?

😊 🤦‍♂️

@bryankeller
Copy link
Contributor Author

It was the least I could do after not getting to the review for 2 months 😅 Thanks for your patience and for implementing this - this is an awesome feature to have, and gets us even closer to feature parody with flow layout!

@bryankeller bryankeller merged commit 2d51e8d into master Apr 9, 2019
@bryankeller bryankeller deleted the evil159/footer-support-rebased branch April 9, 2019 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants