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

Code Enhancements & README #2

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Code Enhancements & README #2

wants to merge 10 commits into from

Conversation

pauljohanneskraft
Copy link
Collaborator

Name of the PR

♻️ Current situation

Describe the current situation (if possible with and exemplary (or real) code snippet and/or where this is used)

💡 Proposed solution

Describe the solution (if possible with and exemplary (or real) code snippet)

Problem that is solved

Provide a description and link issues that are solved

Implications

Describe the implications, e.g. refactoring

➕ Additional Information

Provide some additional information if possible

Related PRs

Reference the related PRs

Testing

Are there tests included? If yes, which situations are tested and which corner cases are missing?

Reviewer Nudging

Where should the reviewer start, where is a good entry point?

@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #2 (fcbea6b) into develop (ea7630c) will increase coverage by 2.28%.
The diff coverage is 25.12%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop       #2      +/-   ##
===========================================
+ Coverage    28.31%   30.60%   +2.28%     
===========================================
  Files          110       90      -20     
  Lines         2165     1866     -299     
===========================================
- Hits           613      571      -42     
+ Misses        1552     1295     -257     
Impacted Files Coverage Δ
Sources/Presenter/Configuration/NamedType.swift 100.00% <ø> (ø)
Sources/Presenter/Configuration/Plugin.swift 37.50% <0.00%> (ø)
Sources/Presenter/Model/Action.swift 100.00% <ø> (ø)
Sources/Presenter/Model/Binding.swift 100.00% <ø> (ø)
Sources/Presenter/Model/CoderAction.swift 69.44% <ø> (-11.12%) ⬇️
Sources/Presenter/Model/ComposedAction.swift 0.00% <ø> (ø)
Sources/Presenter/Model/CopyAction.swift 0.00% <0.00%> (ø)
Sources/Presenter/Model/ModelView.swift 0.00% <0.00%> (ø)
Sources/Presenter/Model/SetAction.swift 0.00% <0.00%> (ø)
Sources/Presenter/Model/State.swift 71.42% <ø> (ø)
... and 99 more

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 ea7630c...fcbea6b. Read the comment docs.

Copy link
Contributor

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

I took a look at the PR and I like the new additions 👍
I had a few comments in the READMe and have a few general comments mostly related to the example:

  • You didn't commit the Xcode projects in the repo as they are part of the .gitignore file
  • You can simplify the structure of the Example app and do not need asset catalogues or info.plist with the new Xcode 13 project format. You can take a look at example in the https://github.com/Apodini/CollectorAnalystPresenterExample repo and can also take inspiration from the setup and the CI testing the client application over there
  • I would move the example web service in the example folder and create a Xcode Workspace to be able to open the server and client apps at the same time. Also similar to the setup found in the https://github.com/Apodini/CollectorAnalystPresenterExample repo.


## Contributing

Contributions to this projects are welcome. Please make sure to read the [contribution guidelines](https://github.com/Apodini/.github/blob/release/CONTRIBUTING.md) first.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Contributions to this projects are welcome. Please make sure to read the [contribution guidelines](https://github.com/Apodini/.github/blob/release/CONTRIBUTING.md) first.
Contributions to this project are welcome. Please make sure to read the [contribution guidelines](https://github.com/Apodini/.github/blob/main/CONTRIBUTING.md) and the [contributor covenant code of conduct](https://github.com/Apodini/.github/blob/main/CODE_OF_CONDUCT.md) first.

}
```

This example above already showcases some of the more complicated features of SwiftUI, including state management and local actions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to show some example code how the View is then transitioned into a data representation just to make this example complete and e.g. show how to define a view, instantiate it and then decode it into a data representation. How this data is then returned (e.g. in a Vapor or Apodini) server is then out of the scope of the README.

```

When using plugins, custom views, custom view modifiers or actions, please make sure to register them during app start using `Presenter.use(plugin:)`, `Presenter.use(view:)`, `Presenter.use(modifier:)` or `Presenter.use(action:)` respectively. Tip: You can also create an app-specific plugin to register multiple views, view modifiers and actions at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to show how to interact with native client code and/or use plugins here but maybe this is then part of a separate .md file somewhere in a Documentation folder and a next time.

@@ -1,18 +1,112 @@
# Presenter

## Requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to also show some badges here similar to https://github.com/Apodini/ApodiniCollector
You can use the following badge for the DOI:
DOI

@PSchmiedmayer
Copy link
Contributor

@pauljohanneskraft Is there an update on this PR and fixing the issues when getting the presenter views of the system in the CollectorAnalystPresenterExample repo?

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.

2 participants