Skip to content

Next 10 Diagrams! - #106

Merged
brianegan merged 21 commits into
masterfrom
30-40
Nov 11, 2019
Merged

brianegan merged 21 commits into
masterfrom
30-40

Conversation

@brianegan

Copy link
Copy Markdown
Contributor

@Piinks Piinks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Apologies for the delay! More awesome stuff! 🎉

Comment thread packages/diagrams/lib/src/divider.dart Resolved
Comment thread utils/diagram_generator/lib/main.dart Resolved
@brianegan

Copy link
Copy Markdown
Contributor Author

Thanks so much for the review! Apologies for the inconvenience, but I started a vacation and I'm out until October 7th.

I'll certainly update this PR when I'm back in action :)

@Piinks

Piinks commented Sep 27, 2019

Copy link
Copy Markdown
Contributor

Thanks so much for the review! Apologies for the inconvenience, but I started a vacation and I'm out until October 7th.

I'll certainly update this PR when I'm back in action :)

No inconvenience at all! Enjoy your vacation! 🏖

… necessary configuration widgets"

This reverts commit 12ef635.
that includes a `navigator` option for `showDialog`, `showDatePicker`, and `showGenericDialog`.
@Piinks

Piinks commented Nov 4, 2019

Copy link
Copy Markdown
Contributor

Hey! I saw flutter/flutter#42842 was merged in! 🎉
Is this ready for another review? Or does it need an update?

@brianegan

brianegan commented Nov 5, 2019 •

Copy link
Copy Markdown
Contributor Author

Hi @Piinks! Yep, I think this one is mostly good to go, but with one question: What kind of extra labels do you think would be good to add to the SingleChildScrolLView diagrams?

I've read through the github issue, linked twitter thread and sample github repo, but I couldn't quite think of the kind of diagrams or animations (mentioned by Ian) that might be helpful to add here that would address the underlying issue -- which seemed more related to SliverFillRemaining.

I can certainly add an appBar to each one, but I wasn't quite sure what to label in the diagram that might help answer those linked questions?

@Piinks

Piinks commented Nov 6, 2019

Copy link
Copy Markdown
Contributor

I can certainly add an appBar to each one, but I wasn't quite sure what to label in the diagram that might help answer those linked questions?

An app bar sounds like a good idea. Perhaps also, we can just add text that labels each box with its associated widget from the composition? WDYT?

@brianegan

brianegan commented Nov 7, 2019 •

Copy link
Copy Markdown
Contributor Author

Sounds good! Since the documentation references "fixed height" vs "expanded" content sections I've used those terms to label the boxes.

Please let me know if those terms work or if you'd like me to use the name Widget itself, such as "Fixed Height Container"

Other than that, I'll watch the build to make sure it passes, and should be good for a final review :)

@Piinks Piinks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!
Thanks for addressing the breaking change in image.dart! 🎉

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.

3 participants