Skip to content

Conversation

@tinkrtailor
Copy link
Collaborator

Adding support in order to be able to animate the fontSize of a text. This does however come at some cost.

Instead of passing:

 Style.({ "headerText": style([fontSize(14.0)]) }

We now will need to pass the float into the style Float constructor Float(14.0) and pass it as such into the fontSize function. So the new way to do this will be:

 Style.({  "headerText": style([fontSize(14.0)]) }

In our project we have a style module so we can define and reuse style variables across all components. So we can update the variable definition for the medium fontSize like from:
let medium = 14.; => let medium = Float(14.); in order to account for this api change.

@tinkrtailor tinkrtailor changed the title modifying fontSize style in order to be able to animated fontSize Modifying fontSize style in order to be able to animate fontSize Dec 13, 2017
@wokalski
Copy link
Collaborator

I'm good with merging this but I'm thinking that you could easily add a migration script here based on the one I wrote. It'd be quite easy but my only concern is going down the rabbit hole of sharing infra between the scripts.

@wokalski
Copy link
Collaborator

One quick question, could you remove the last commit? It's not really necessary I think 👍.

@MoOx
Copy link
Member

MoOx commented Dec 14, 2017

@wokalski "Squash and merge" are our best friend in this kind of case imo :)

@tinkrtailor
Copy link
Collaborator Author

I'm a bit reluctant to write a separate upgrade script for this, this being a small change and can be easily adjusted to most projects with sed. But I could add this change to the update script you did so that people migrating to the new styles api would get the correct syntax for this as well. How does that sound? @wokalski

@wokalski
Copy link
Collaborator

@gunnigylfa I think we're good. However in the future I'm planning to make it a mandatory patch for every breaking change. Alright, merging!

@wokalski wokalski merged commit a293beb into master Dec 14, 2017
@wokalski wokalski deleted the adding-animation-support-for-font-size branch December 14, 2017 13:39
@arnarthor
Copy link
Collaborator

I think it should be a separate script since this will be a new breaking change to those who've already updated to 0.4.0. This will need to be 0.5.0 release

@wokalski
Copy link
Collaborator

@arnarthor this change is not super substantial, that said if we went that far there's no coming back 😄. We need some infra for those scripts to be able to create them easily.

@arnarthor
Copy link
Collaborator

Oh yeah, I didn't see your answer before I wrote the comment. I don't think an update script is needed for this, but if we were to do it, have it as a separate one, not included in the other.

That said, we can just ship this as 0.5.0 and ask people to follow the compile errors since it should only affect fontSize attributes

@wokalski
Copy link
Collaborator

we need: npm run bs-react-native-upgrade but I'm not sure how to handle versioning here. A lock file somewhere? Not quite sure.

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.

5 participants