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

Publish app architecture 'case-study' pages #11414

Merged
merged 51 commits into from
Dec 2, 2024

Conversation

ericwindmill
Copy link
Contributor

@ericwindmill ericwindmill commented Nov 19, 2024

This is the second of two large PRs that add architecture guidance to the website.

The code snippets are from compass app in flutter/samples. It was been thoroughly reviewed.

Resolves third task from #11374
Fixes #11432

Presubmit checklist

  • This PR is marked as draft with an explanation if not meant to land until a future stable release.
  • This PR doesn’t contain automatically generated corrections (Grammarly or similar).
  • This PR follows the Google Developer Documentation Style Guidelines — for example, it doesn’t use i.e. or e.g., and it avoids I and we (first person).
  • This PR uses semantic line breaks of 80 characters or fewer.

@ericwindmill ericwindmill changed the title Ew app architecture case study Publish app architecture 'case-study' pages Nov 19, 2024
@flutter-website-bot
Copy link
Collaborator

flutter-website-bot commented Nov 19, 2024

Visit the preview URL for this PR (updated for commit d46134f):

https://flutter-docs-prod--pr11414-ew-app-architecture-case-st-5j24ywro.web.app

@RobertBrunhage
Copy link

General question, might not be the correct place.

Are you also looking at adding this to the skeleton template? That is currently in the recommended resources and doesn't follow the MVVM approach.

@ericwindmill
Copy link
Contributor Author

General question, might not be the correct place.

Are you also looking at adding this to the skeleton template? That is currently in the recommended resources and doesn't follow the MVVM approach.

Yes, I'm going to update skeleton soon! That's next on my list for this project

@ericwindmill ericwindmill marked this pull request as ready for review November 20, 2024 18:14
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks for making those initial adjustments @ericwindmill! And sorry about the delay on this second review.

I've gone through and left a few more comments as well as a few questions. Note that some might require modifications across the page or a few pages.

A few smaller comments were mostly about following the style guide, and in those cases I tried to link back to the relevant guidelines.

I don't have the entire context of the app architecture work, so please feel free to push back on anything that doesn't make sense or you'd prefer to complete in follow up work.

Thanks again :D

src/_data/sidenav.yml Show resolved Hide resolved
src/_data/sidenav.yml Outdated Show resolved Hide resolved
src/_data/sidenav.yml Outdated Show resolved Hide resolved
src/content/app-architecture/case-study/index.md Outdated Show resolved Hide resolved
src/content/app-architecture/case-study/index.md Outdated Show resolved Hide resolved
src/content/app-architecture/case-study/ui-layer.md Outdated Show resolved Hide resolved
src/content/app-architecture/case-study/ui-layer.md Outdated Show resolved Hide resolved
src/content/app-architecture/case-study/ui-layer.md Outdated Show resolved Hide resolved
src/content/app-architecture/case-study/data-layer.md Outdated Show resolved Hide resolved
src/content/app-architecture/case-study/data-layer.md Outdated Show resolved Hide resolved
@parlough parlough removed their assignment Nov 27, 2024
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all of my comments and questions @ericwindmill! I've made a few more minor changes and fixes, but in general this looks ready to land so you can keep iterating.

Great work on this gigantic section of documentation that will definitely help new developers understand and implement the recommendations :D

Once most of the architecture work has landed, I'll go through the architecture section again as a whole to complete any further cleanup and adjustments.

Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

Hooray! Thanks for your review @parlough and thanks for your edits @miquelbeltran and @ericwindmill! I, too, want to review everything when it's in place. But for now, I land!

@sfshaza2 sfshaza2 merged commit 0cddf67 into main Dec 2, 2024
9 checks passed
@sfshaza2 sfshaza2 deleted the ew-app-architecture-case-study branch December 2, 2024 15:29
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.

The ViewModel resides in the Logic layer
5 participants