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

Style standardisation #14

Merged
merged 8 commits into from
Oct 18, 2017
Merged

Style standardisation #14

merged 8 commits into from
Oct 18, 2017

Conversation

bolismauro
Copy link
Contributor

@bolismauro bolismauro commented Oct 18, 2017

Changes:

  • Added helpers for style standardisation
  • Updated README with conventions
  • Added bonmot helpers with related pod

Please check the readme carefully as it contains most of the work.

This PR doesn't provide any automation process from Sketch, but it creates a foundation for it. The idea is to start adopting this style and be ready for the automation step we will face in the (hopefully near) future

@cipolleschi
Copy link
Contributor

Uhm... in general i like the approach but:

  1. not all of us uses Sketch. In Delta, for instance, all our designs are only in InVision
  2. the Style file will become very big, especially with big apps that have several views. We used a similar approach in Squat and ChatStories, but, at the end, the file was very cluttered and very hard to find what we were searching for.

Right now, in Yoga, we resolved to use a single file for the shared styles (e.g.: the navigation bar style) and, inside the UI folder, for each screen folder, we create a Screen+Style file in which we put the style of the Labels for that screen

@m4tbat
Copy link
Contributor

m4tbat commented Oct 18, 2017

One thing that's not clear to me is: why should we do this:

Tempura.applyStyle(Style.Layer.redView, to: self.backgroundView)

instead of simply doing this:

Style.Layer.redView(self.backgroundView)

?

@bolismauro
Copy link
Contributor Author

bolismauro commented Oct 18, 2017

RC

not all of us uses Sketch. In Delta, for instance, all our designs are only in InVision

well, I think it is suboptimal, but you can still do it. There is nothing that prevents you not use Sketc

the Style file will become very big, especially with big apps that have several views. We used a similar approach in Squat and ChatStories, but, at the end, the file was very cluttered and very hard to find what we were searching for.

You can create multiple extensions of Text, I don't think it is a problem (it will become when/if we will automate the process)

@madbat I prefer it too, but it is quite hard / impossible to implement. If you want to try let me know

@m4tbat
Copy link
Contributor

m4tbat commented Oct 18, 2017

@bolismauro 🤔 maybe I'm missing something.

This example function from the README

 extension Style.Layer {
   static func redView(_ view: UIView) {
     view.backgroundColor = .red
   }
 }

shouldn't it just be possible to use it like this inside my view's style method:

Style.Layer.redView(self.backgroundView)

?

@smaramba
Copy link
Contributor

smaramba commented Oct 18, 2017

@madbat It is indeed possible, one reason not to do that is to have a Tempura function that is always invoked when styling, in case we want to do something later on...
My opinion is that Style.Layer.redView(self.backgroundView) is better for the time being, given that the other approach would introduce asymmetry in the styling of Text and Layers.
This means that all the code changes in this PR needs to be reverted and we only add the recommendations in the README.
@bolismauro

Remove ApplyStyle
@smaramba smaramba merged commit 1f5777e into master Oct 18, 2017
@smaramba smaramba deleted the feature/style-standardization branch October 18, 2017 15:13
@smaramba
Copy link
Contributor

  • removed the Tempura.applyStyle function
  • updated the README
  • fixed TempuraHelpers tests

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