Skip to content

Move flags icon to main project.#11367

Merged
jklausa merged 11 commits intodevelopfrom
feature/11361-move-flags-icon-to-main-project
Apr 15, 2019
Merged

Move flags icon to main project.#11367
jklausa merged 11 commits intodevelopfrom
feature/11361-move-flags-icon-to-main-project

Conversation

@jklausa
Copy link
Contributor

@jklausa jklausa commented Mar 28, 2019

Fixes #11361.

This moves (well, copies.) all the flags images back to the main app.

Long-term goal is to get rid of WPStatsiOS framework entirely, so we don't want to rely on those images living in the framework when switching to the new data architecture.

To test:
Go to Stats > Period.
Verify that the flags are still displayed correctly.

@jklausa jklausa added this to the 12.2 milestone Mar 28, 2019
@jklausa jklausa self-assigned this Mar 28, 2019
@jklausa jklausa requested a review from a user March 28, 2019 18:12
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

"images" : [
{
"idiom" : "universal",
"filename" : "AC.png",
Copy link

Choose a reason for hiding this comment

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

Do we have vector or higher-resolution versions of these assets available?

Copy link

Choose a reason for hiding this comment

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

It appears that these images are 72px square, effectively 24pts on larger devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great question!

@SylvesterWilmott do you know if we have those somewhere and if so, how could I obtain them?

@ghost
Copy link

ghost commented Mar 28, 2019

Would it be possible to add tests to ensure that these asset catalog entries can all be loaded successfully?

@@ -8719,6 +8722,7 @@
98B11B8B2216536C00B7F2D7 /* StatsChildRowsView.xib in Resources */,
2FAE970C0E33B21600CA8540 /* xhtml1-transitional.dtd in Resources */,
4058F41A1FF40EE1000D5559 /* PluginDetailViewHeaderCell.xib in Resources */,
406A0EF0224D39C50016AD6A /* Flags.xcassets in Resources */,
Copy link

Choose a reason for hiding this comment

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

Should this diff also be accompanied by 258 deletions, reflecting removal of the standalone PNGs from WordPressComStatsiOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, since we're gonna ship the new stats in the next release and those are required to remain in place for the "old" Stats to continue to work.

@jklausa
Copy link
Contributor Author

jklausa commented Mar 28, 2019

Would it be possible to add tests to ensure that these asset catalog entries can all be loaded successfully?

Great point.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hi @jklausa - I left a few comments. I think this is headed in the right direction, but there are a few things that give me pause.

  1. It appears that we've moved (copied) assets, essentially doubling the footprint of our image assets.
  2. It appears that a "hack" method mentioned in the comments can be removed in favor of the correct solution.

At a minimum, I'd like to see that either (a) these items are addressed before we merge the PR; or (b) we create new issues that allow us to track & address them subsequently.

I also think there's an opportunity to work with design to create higher-resolution assets that will enable to put our best foot forward once our refreshed Stats feature launches, but acknowledge that that may be outside the scope of this PR.

cc: @SylvesterWilmott - do we have these image assets (i.e., country flags) available at higher resolutions?

@jklausa
Copy link
Contributor Author

jklausa commented Mar 29, 2019

I updated this, removing the "hack" and adding tests to make sure that the images can be loaded correctly.

I think removing the "old" assets is captured by #10975 — when we remove WordPressComStatsiOS, we'll get rid of the other copy too :)

I'm happy to let this PR wait here for a while if @SylvesterWilmott can point us to a higher-resolution assets.

@SylvesterWilmott
Copy link

SylvesterWilmott commented Mar 29, 2019

@stevebaranski is there a reason we need higher resolution flags? The flag icons should only ever appear at 24pt size so the 72pt icons are perfect for 3x displays.

I would actually even encourage Android to adopt the flag icons we use in iOS @planarvoid

@ghost
Copy link

ghost commented Mar 29, 2019

is there a reason we need higher resolution flags? The flag icons should only ever appear at 24pt size so the 72pt icons are perfect for 3x displays.

@SylvesterWilmott I appreciate that the dimension specified in Zeplin is 24pt square. If we'd like to afford ourselves flexibility in the future, now is the time to make such a change. I'd rather ask now than never! 😄

I was most concerned about the manner with which they were imported into the asset catalog. For example:

Screen Shot 2019-03-29 at 1 32 02 PM

Note that the base image was imported into the "1x" cell within the asset catalog. This has implications when a build is created for the App Store, and then thinned for each device, especially when we're vending raster assets.

@jklausa - based on Sylvester's input here, I think these images should be placed in the 3x cell for each asset. Agreed?

@ghost
Copy link

ghost commented Mar 29, 2019

@jklausa once this is merged, we might want to advise our platform counterparts that we anticipate a slight uptick in the app footprint due to these extra images.

@jklausa jklausa modified the milestones: 12.2, 12.3 Apr 8, 2019
@jklausa
Copy link
Contributor Author

jklausa commented Apr 15, 2019

@stevebaranski argh, I forgot about this PR to death. I managed to wrangle a neat vim macro that let me update all the files in one go and I added a test to make sure the images are loading in a correct size/scale.

I think we should be good to ship this now :)

@jklausa jklausa requested a review from a user April 15, 2019 02:10
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hi @jklausa - yes indeed, this looks good to me now! :shipit:

@jklausa jklausa merged commit 448563c into develop Apr 15, 2019
@jklausa jklausa deleted the feature/11361-move-flags-icon-to-main-project branch April 15, 2019 15:44
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.

Move Flag icons from WPStatsiOS to the main project.

2 participants