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

Create README.md #30

Merged
merged 37 commits into from Nov 11, 2016
Merged

Create README.md #30

merged 37 commits into from Nov 11, 2016

Conversation

smaramba
Copy link
Contributor

@smaramba smaramba commented Nov 3, 2016

@bolismauro @lucaquerella here the first draft of the readme.
I've looked at pretty much every readme of every significant swift framework, this is (excluding the copy that is work in progress) my best summary of what a readme should contain.
feel free to suggest or question everything.

my suggestion on how to improve this:

  • improve copy
  • insert/update the items in the roadmap (under features)
  • introduce also the advanced stuff (i.e. asyncActions)
  • tune the example to something comprehensive that we will also release as a complete example
  • include more examples, even complex ones

@smaramba smaramba mentioned this pull request Nov 3, 2016

## Overview

### defining the logic of your app
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd start with a very basic snippet that users can copy/paste and try katana (the UI probably).
20 lines max, something very simple

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use Katana elements of course

you can ask the `Store` to be notified for every change in the app state

```swift
store.addListener() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd avoid to use this example. It is really something we don't want to see in applications :)


## Overview

### defining the logic of your app
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd start with the UI, since it is the thing you see first.
It is also easier to explain. "this is how you can create the UI, need the logic? ok let's add it"

@smaramba
Copy link
Contributor Author

smaramba commented Nov 3, 2016

after a discussion with @bolismauro we decided the things to change for the next iteration:

  • move the feature section at the end, changing name to 'roadmap'
  • create a small complete app (max 20 lines of code) as a first thing of the quick start section

@smaramba
Copy link
Contributor Author

smaramba commented Nov 3, 2016

after a discussion with @lucaquerella we decided that:

  • we need to lower down the references to redux/react
  • remove the store.addListener reference
  • remove the table from the code snippets

@smaramba smaramba force-pushed the feature/readme branch 2 times, most recently from bf4f292 to b79139e Compare November 7, 2016 16:03
@smaramba
Copy link
Contributor Author

smaramba commented Nov 8, 2016

  • moved Roadmap to the end
  • only one reference to redux/react
  • removed the addListener { table.reloadData() }
  • changed the quick start to not include table

@bolismauro
Copy link
Contributor

bolismauro commented Nov 8, 2016

Some feedback

  • Titles should be uppercase
  • I'd change in a Katana app all the state is entirely described by a single serializable data structure and the only way to change the state is to emit an action. in is to dispatch an action. Dispatch in general is the correct verb (and the one we use in the implementation)
  • UI: in a Katana app you define the UI in terms of a tree of components declaratively described by props (external world) and state (internal world). This approach lets you think about components as an isolated, reusable piece of UI, since the way a component is rendered only depends on the current props and state of the component itself. I'm not sure if we need to push more on the no mutation thing here
  • <3 heart smile here :D
  • In Katana you declaratively define your UI components through NodesDescriptions. Not sure here how to put the protocol name plural, but NodesDescriptions is not correct in my opinion
  • When it's time to render the component, the method applyPropsToNativeView is called, this is where we need to adjust our nativeView to reflect the props and the state. It is not needed for this example. Should we skip it? If we want to show the applyPropsToNativeView maybe we can find another way? (e.g., background color?)
  • add a section how to contribute. I would mention PR and maybe swiftlint

@smaramba
Copy link
Contributor Author

smaramba commented Nov 9, 2016

  • uppercase titles
  • emit -> dispatch
  • <3 -> ❤️ 
  • NodeDescriptions -> NodeDescription (updated copy)
  • explained that applyPropsToNativeView is not necessary in the example
  • created a Contribute section, we should create a CONTRIBUTE.md file with all the details. I'm opening an issue for that

@smaramba
Copy link
Contributor Author

smaramba commented Nov 9, 2016

  • removed repetitions of the in katana... pattern inside the bullet list at the beginning
  • added reference to plastic
  • added bullet list to explain the StateType, PropsType and NativeView
  • updated the example with the Renderer

@bolismauro
Copy link
Contributor

  • In the Defining the UI section
    • first snippet. We need to add Keys that has been added by Refactor animations #35
    • second snippet. We need also alpha and key. NodeProps is changed to NodeDescriptionProps
    • third snippet. We decided to use the typealias in the signature (I'd stay consistent with the examples). We also need to set the alpha to the native view
    • fourth snippet. We decided to use typealias in the signature
  • in the Attaching the UI to the Logic section
    • first snippet. We decided to use typealias in the signature
  • In the Layout of the UI
    • first snippet. We decided to use typealias in the signature
    • I'd also add (again) a Plastic link. Something like "for more information about the philosophy...". We can decide to add it at the bottom (e.g., in where to go from here)
    • let rootView = views.nativeView I'd call it nativeView

Suggestions for roadmap:

  • Declarative table (link feature as soon as it is available)
  • MacOS (link feature as soon as it is available)
  • Improve testing
  • Expand documentation

Besides these points, SGTM

@bolismauro
Copy link
Contributor

@smaramba I'd also remove the StackOverflow thing at least at the beginning.
We can use issues to start with

@bolismauro
Copy link
Contributor

@smaramba remember to update the headers according to #50

@lucaquerella
Copy link
Contributor

lucaquerella commented Nov 9, 2016

Great job here. <3

Suggestions:

  • Remove <-> in the first section (you can use ↔️ for example)
  • For coolness sake add a table with the pro of using katana - similar to what you can find here: https://github.com/fastlane/fastlane (Suggested by Dr. Bolis)
  • Add a section where you describe the animations, I'd add a gift that shows the Pokemon example

@lucaquerella
Copy link
Contributor

lucaquerella commented Nov 9, 2016

@marcopaz (aka Paz) make me notice that the logo is not high resolution. can we fix it?
he also spotted a typo:

logic: app all the state is entirely described

@smaramba
Copy link
Contributor Author

yeah I'm asking tomasz to give us the @2x version

@smaramba
Copy link
Contributor Author

  • moved the Installation section under Overview
  • Removed <-> in the first section (used ↔️ instead)
  • removed typo logic: app all the state ...
  • added alpha and keys to the props
  • renamed NodeProps -> NodeDescriptionProps
  • enforced the typealiases inside the applyPropsToNativeViewmethod signature
  • added alpha to applyPropsToNativeView
  • enforced the typealiases inside the childrenDescriptions method signature
  • enforced the typealiases inside the connect method signature
  • enforced the typealiases inside the layout method signature
  • added reference to Plastic even in the Layout of the UI section
  • renamed rootView -> nativeView
  • included in the roadmap the suggestions from @bolismauro
  • removed the stackoverflow section

TODO:

  • add table with PRO (in progress)
  • add @2x logo (waiting asset from tomasz)
  • introduce animations (including pokemon example) (in progress)


## Contribute

- If you __found a bug__, open an issue

Choose a reason for hiding this comment

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

If you found -> If you've found (once again, it doesn't say when the bug was found)

Copy link

@mdanieli mdanieli left a comment

Choose a reason for hiding this comment

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

Added comments and fixes.

| :muscle: | Use support for middleware like Logging |
| :tophat: | Automatically update the UI when your app state changes |
| :triangular_ruler: | Automatically scale your UI to every size and aspect ratio |
| :horse_racing: | Animate all the UI changes |
Copy link
Contributor

Choose a reason for hiding this comment

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

What about Easily animate UI changes?
I'd also add something about the one way data flow. I think it is important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

labelProps.key = CounterScreen.Keys.label.rawValue
labelProps.textAlignment = .center
labelProps.backgroundColor = .mediumAquamarine
labelProps.text = NSAttributedString(string: "Count: \(props.count)", attributes: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just use labelProps.text = NSAttributedString(string: "Count: \(props.count)")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@bolismauro bolismauro merged commit 9807029 into master Nov 11, 2016
@bolismauro bolismauro deleted the feature/readme branch November 11, 2016 16:39
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.

None yet

4 participants