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

Consume Sub BannerText in InstructionView #1408

Merged
merged 1 commit into from
Oct 12, 2018
Merged

Consume Sub BannerText in InstructionView #1408

merged 1 commit into from
Oct 12, 2018

Conversation

danesfeder
Copy link
Contributor

Closes #1228

ezgif com-video-to-gif

Nothing here that large portions of the InstructionModel and InstructionStepResources are being created / calculated for the deprecated InstructionView#update method. Does it make sense to remove the method, break the API and remove the unnecessary model work?

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Nothing here that large portions of the InstructionModel and InstructionStepResources are being created / calculated for the deprecated InstructionView#update method. Does it make sense to remove the method, break the API and remove the unnecessary model work?

Yes please 🙏 Let's remove the unnecessary code!

I think it's fine as soon as we ensure that the release notes and the android docs site are updated with these breaking changes 🚀

Other than that ☝️ this looks good to me.

Thanks @danesfeder

@danesfeder danesfeder added the backwards incompatible Requires a SEMVER major version change. label Oct 12, 2018
@danesfeder
Copy link
Contributor Author

@Guardiola31337 this is updated and ready for another round 👀

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Love that +139 -292

+139 -292

Thanks @danesfeder :shipit:

@danesfeder danesfeder merged commit 6fbf3ce into master Oct 12, 2018
@danesfeder danesfeder deleted the dan-sub branch October 12, 2018 17:40
@danesfeder danesfeder mentioned this pull request Oct 24, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible Requires a SEMVER major version change. bug Defect to be fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants