Skip to content
This repository has been archived by the owner on Aug 16, 2019. It is now read-only.

Added json example to the example application #11

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mathiasAichinger
Copy link
Contributor

No description provided.

@mathiasAichinger
Copy link
Contributor Author

Used open-source icons for the example (https://github.com/iconic/open-iconic). Should it be mentioned somewhere?

@joanromano
Copy link
Contributor

SwiftLint found issues

Warnings

File Line Reason
ComponentMeta.swift 78 Line should be 100 characters or less: currently 103 characters

Generated by 🚫 danger

@codecov-io
Copy link

codecov-io commented Jan 19, 2017

Codecov Report

Merging #11 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #11   +/-   ##
=======================================
  Coverage   99.01%   99.01%           
=======================================
  Files           9        9           
  Lines         306      306           
=======================================
  Hits          303      303           
  Misses          3        3

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 842a6bf...06e62df. Read the comment docs.

@MP0w
Copy link
Contributor

MP0w commented Jan 20, 2017

@joanromano did you use your account as danger bot? ahaha da func, should be replaced by something that is clearly a bot... like @devluckybot 😬

@joanromano
Copy link
Contributor

joanromano commented Jan 20, 2017

Apparently we can't configure with a bot since it's from buddybuild. Also seems like they have an old swiftlint version installed, either we add the default value on our swiftlint file or we add a pre build phase http://docs.buddybuild.com/docs/custom-prebuild-and-postbuild-steps

@mathiasAichinger could you look into that before merging? Added the pre build script on my PR and now uses the last swiftlint version also in the app example, check it here #12

Copy link
Contributor

@joanromano joanromano left a comment

Choose a reason for hiding this comment

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

I would also add a README.md explaining what this example is trying to show, e.g loading the main component from a JSON which is a component of type X and one of the childs, of type Y, is loaded directly from code... also you can mention here about the open source icons.

factory.register(with: "matrioska", factoryBuilder: matrioskaCodeBuilder)

do {
if let json = try JSONReader.jsonObject(from: "app_structure"),
Copy link
Contributor

Choose a reason for hiding this comment

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

JSONFactory should take care of app_structure I guess, will update that with the schema documentation.


let stackBuilder: JSONFactory.ClusterFactoryBuilder = { (children, meta) in
ClusterLayout.stack(children: children, meta: meta)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Or let stackBuilder: JSONFactory.ClusterFactoryBuilder = ClusterLayout.stack

- Pods
- Example

reporter: "xcode"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the main .swiftlint file?

@testable import Matrioska

/// A JSONReader used to convert to JSONObject
final class JSONReader {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't duplicate this file here, will add structure to the JSONFactory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah too bad we cannot use the one from the project.

axis: .vertical,
preserveParentWidth: true,
backgroundColor: .blue)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not returning this component when declaring the matrioskaCodeBuilder on AppDelegate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to separate it from the AppDelegate to show that you can always use Matrioska in own ViewControllers


required init?(meta: ComponentMeta?) {
super.init(nibName: nil, bundle: nil)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this init?


let hexColor = meta["color"] as? String
let color = UIColor(hexString: hexColor ?? "")
self.color = color
Copy link
Contributor

Choose a reason for hiding this comment

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

Or just color = UIColor(hexString: hexColor ?? "")

@MP0w
Copy link
Contributor

MP0w commented Feb 24, 2017

@ChristianOrgler
Copy link

@mathiasAichinger what's the state of this?

@mathiasAichinger
Copy link
Contributor Author

@ChristianOrgler from my side it's ready for review. Removed the icons from the iconic project and replaced with 2 self drawn.

Copy link
Contributor

@joanromano joanromano left a comment

Choose a reason for hiding this comment

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

I think travis is using an old version of Swiftlint because it's complaining about a 100 line length (which is the previous default value, now is 120). @mathiasAichinger could you have a look before merging (it's also not passing the checks somehow).

Also, please add a README.md file in the root folder of the Example explaining what this example is trying to show, e.g loading the main component from a JSON which is a component of type X and one of the child components, of type Y, is loaded directly from code...

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.

5 participants