Skip to content
This repository has been archived by the owner on Jan 25, 2025. It is now read-only.

Added API to remove and insert children #10

Merged
merged 4 commits into from
Apr 26, 2020

Conversation

samschott
Copy link
Member

@samschott samschott commented Feb 15, 2020

This PR adds an API to insert and remove children to / from a node.

Edit: The changes suggested here form the bases for PR #814 in toga.

@samschott samschott force-pushed the remove-and-insert-api branch from b8bbbc5 to a4c6f71 Compare February 15, 2020 22:53
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This looks fine, and the reasoning around the need to process children makes sense.

What is missing is tests to validate the behavior. add() is undertested at present (which is at least part of the reason that this problem appeared in the first place); but if we're adding APIs that push that aspect of layout extensively, we should add tests to validate everything works the way it should.

@samschott samschott force-pushed the remove-and-insert-api branch from 8e7fe0a to b2cdac9 Compare February 16, 2020 11:54
@samschott
Copy link
Member Author

Agreed. I have added simple checks to test_node for now but it may be good to add tests in test_layout for modifying children post-creation.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks good; I'm not sure I follow about what specific tests you think could be added around modifying children after insertion; I'm pretty sure those should be covered by existing style mutation checks.

@freakboy3742 freakboy3742 merged commit a9d123e into beeware:master Apr 26, 2020
freakboy3742 added a commit that referenced this pull request Apr 26, 2020
@samschott
Copy link
Member Author

I was planning to still add a test for dimensions to be updated when a new child is inserted before existing ones. This was not working properly before this PR and wasn't caught be tests because there was no insert functionality.

I'll have to look into how the style tests actually work and may open another PR for this.

@samschott samschott deleted the remove-and-insert-api branch April 26, 2020 08:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants