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

Update WPMediaPicker from version 1.7.2 to 1.8.0-beta.1 #17584

Merged
merged 2 commits into from
Dec 2, 2021

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Nov 29, 2021

This is a routine update prompted by a step in our release process checklist.

@jkmassel @joshheald @frosty you folks are the last to have interacted with the library. Jeremy and Josh on Woo and the lib itself and Frosty here, 0b3c392. Can you tell me if there's a reason why this was never update, and no one raised an issue about it?

Two possibilities:

  • I missed it since its release, or like in the previous release cycle, simply run out of time for this
  • There's no reason to upgrade, so no dev did it

If it's the latter, then feel free to close this PR.

I also noticed @SergioEstevao bumped the library to 1.8.0-beta.1 (although, there's no tag for it, yet). So let me know if it would be better to use that beta here instead.

Regression Notes

  1. Potential unintended areas of impact
    The media picking workflow, I guess? The version has been out for a while and used in Woo, so it should be safe-ish to adopt here, too.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N.A.

  3. What automated tests I added (or what prevented me from doing so)
    N.A.


  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes. N.A.
  • I have considered adding accessibility improvements for my changes. N.A.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.

@mokagio mokagio added the Tooling Build, Release, and Validation Tools label Nov 29, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 29, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

@mokagio mokagio added this to the 18.9 milestone Nov 29, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 29, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@mokagio mokagio marked this pull request as ready for review November 29, 2021 05:44
@joshheald
Copy link
Contributor

@jkmassel @joshheald @frosty you folks are the last to have interacted with the library. Jeremy and Josh on Woo and the lib itself and Frosty here, 0b3c392. Can you tell me if there's a reason why this was never update, and no one raised an issue about it?

The update I made to the library was to avoid a crash when accessibilityLabel was requested for media which we show just as files. That label is based on filename on the asset, but that's an optional part of the asset protocol and previously, there was no check to see whether it's implemented before calling filename, which would result in a crash when using VoiceOver and other assistive technologies (and possibly even if you're not using them, I can't remember for sure.)

I checked the WPiOS app, and it wasn't susceptible to this crash because it implements filename on its asset subclass, so I couldn't see a pressing need to update. Should I have raised an issue regardless here, do you think? I don't remember seeing anything in the checklist for that, but perhaps the checklist was written from the perspective of someone who's working on the WPiOS app finding an issue, fixing it, and so updating the library version in WPiOS was implied...

@frosty
Copy link
Contributor

frosty commented Nov 29, 2021

Sergio and I opted for 1.8.0 for his set of changes as they bumped the minimum supported version of iOS, so we felt it warranted a bigger version bump than just a bugfix release. I'm not sure why this was replaced by 1.7.3, however?

@mokagio
Copy link
Contributor Author

mokagio commented Nov 30, 2021

@joshheald

I don't remember seeing anything in the checklist for that, but perhaps the checklist was written from the perspective of someone who's working on the WPiOS app finding an issue, fixing it, and so updating the library version in WPiOS was implied...

Nah, that's okay. I think it should be up to the WP iOS devs and us release train conductors to monitor and update the libraries as they change, if necessary. I mean, it would not have been wrong for you to open a PR to update it here, too, since the change was safe, but we can't expect library authors to updated every downstream consumer. It doesn't scale.

@frosty

Sergio and I opted for 1.8.0 for his set of changes as they bumped the minimum supported version of iOS, so we felt it warranted a bigger version bump than just a bugfix release. I'm not sure why this was replaced by 1.7.3, however?

🤔 I went for 1.7.3 because that's what the pod update command resolved.

I think both "issues" are due to this repo not being part of the list we (I?) check a part of the code freeze. I'll make sure to add it from now on. 😅

@mokagio mokagio force-pushed the update-wpmediapicker branch from fa4102f to 82105c6 Compare November 30, 2021 05:53
@mokagio mokagio changed the title Update WPMediaPicker from version 1.7.2 to 1.7.3 Update WPMediaPicker from version 1.7.2 to 1.8.0-beta.1 Nov 30, 2021
This is a routine update prompted by a step in our release process checklist.

@jkmassel @joshheald @frosty you folks are the last to have interacted with the
library. Jeremy and Josh [on
Woo](woocommerce/woocommerce-ios#5138) and [the lib
itself](wordpress-mobile/MediaPicker-iOS@1.7.2...1.7.3)
and Frosty here, 0b3c392. Can you tell me if there's a reason why this was
never update, and no one raised an issue about it?

Two possibilities:

- I missed it since its release, or like in the previous release cycle, simply run out of time for this
- There's no reason to upgrade, so no dev did it

If it's the latter, then feel free to close this PR.

I also noticed @SergioEstevao [bumped](https://github.com/wordpress-mobile/MediaPicker-iOS/blob/56371391da9fc7ea1350b9808af8b4d2f67ecbfd/WPMediaPicker.podspec#L5) the library to 1.8.0-beta.1 (although, there's no tag for it, yet). So let me know if it would be better to use that beta here instead.
@mokagio mokagio force-pushed the update-wpmediapicker branch from 82105c6 to d5357a1 Compare November 30, 2021 21:32
@mokagio
Copy link
Contributor Author

mokagio commented Dec 1, 2021

@frosty @SergioEstevao @joshheald I bumped the library to 1.8.0-beta.1, can you have another look please? 🙏 🙇‍♂️

@mokagio mokagio enabled auto-merge December 1, 2021 01:41
@mokagio mokagio merged commit 30ed540 into develop Dec 2, 2021
@mokagio mokagio deleted the update-wpmediapicker branch December 2, 2021 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tooling Build, Release, and Validation Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants