-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Jetpack App Migration] Update App Banner Branding #67013
Conversation
With this commit, two separate banners are created for both the WP and JP apps. The JP banner will be displayed when the 'jetpack/app-branding' feature flag is set to true (currently only for development builds and wpcalypso). With this initial commit, both banners are the same as the current WP banner. The JP banner will be tweaked with future commits.
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~44 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~66 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~48318 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
20c2864
to
b64b8b1
Compare
By replacing the 'react-lottie-player' dependency with the 'official' 'lottie-web' dependency, it's hoped that there will be a positive impact on performance.
The Jetpack-specific banner is styled via the use of a new '.jetpack' selector. This selector will eventually be removed when it's time for the Jetpack banner to be rolled out to users (along with the removal of any code related to the WordPress banner).
Reposition component for overall readability
b64b8b1
to
02511ed
Compare
👋 @griffbrad, non-urgent ping here. I'm currently working on an update to the app banner displayed on the mobile web. As per p9oQ9f-1jx-p2, I'm experimenting with using I saw you mention at p9oQ9f-1jx-p2#comment-1985 that there shouldn't be as notable a difference if "you’re properly async loading the dependency", which leads me to believe I'm doing something wrong here. Would you perhaps be able to point me to any examples or tips for getting round this? Forgive my ignorance, I've done some digging but it's not immediately clear to me what needs to be done. Thanks in advance for any insight! 🙇♀️ |
I should have been a bit more specific there: having that increase under “Async-loaded Components” is actually exactly what you’re aiming for! The concerning outcome would be the increase showing up under “App Entrypoints” or “Sections” because that would imply the code was being loaded for all users navigating to those sections rather than async when it was actually needed. |
The unit test associated with the app banner was failing with the following error: "TypeError: Cannot set property 'fillStyle' of null" As lottie-web uses canvas and jest doesn't handle canvas by default, it's necessary to install canvas as a dependency to prevent broken unit tests.
36a34f3
to
6c7a31f
Compare
To determine whether an RTL locale is in use, we're currently using the 'useRtl' hook. By replacing the hook with the 'withRtl' HOC, we're able to determine whether a locale is RTL directly within the class component, enabling 'isRTL' to be more cleanly passed down as a prop from the main banner component. This commit removes the call to the 'useRtl' hook from BannerIcon (child) component. Its 'icon' prop will be defined in future commits.
b202b93
to
451ba1c
Compare
As it's not necessary to load the code for both the Jetpack and WordPress banners, this commit separates them out into separate functions. They'll now only be called when necessary.
7b97dd2
to
e385f51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes LGTM -- I only tested one of the banners, but it worked well.
I think the main thing to consider moving forward is that this still adds almost 50KB just to the banners. Since we're on mobile, I think we can expect folks to have poor network connections sometimes, and that 50KB could make it more difficult for the banners to render expediently. That could theoretically cause a drop-off in the number of people clicking through the banner.
Just wanted to throw the idea out there as you keep an eye on any stats related to this after deploying it! :)
I guess the bundle will immediately get bigger in production, even if the Jetpack changes aren't enabled yet. So you should be able to see if there's any impact pretty quickly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 🎉 I left a few comments to consider, but none are likely blockers.
Update: I also meant to share I also tested all the banners, everything worked great. 👏🏻
To avoid any unexpected outcomes and address linter warnings, following feedback, this commit lists the dependency for the useEffect hook. Co-authored-by: David Calhoun <[email protected]>
Co-authored-by: David Calhoun <[email protected]>
As the wrappers around the 'AppBanner' component were drifting far to the right and becoming hard to parse, this commit makes use of 'compose' to group all the wrappers together and improve readability.
The current padding in the banner's text elements causes short lines on smaller screens, which can looks off putting. This commit tweaks the CSS to remove the padding, instead adding a max-width to avoid longer lines (in addition to centring via margin: auto).
As per this comment, it looks like there are some errors associated with the 'canvas' dependency: #67013 (comment) We're also only adding this dependency for the Jest testing environment. As such, this commit replaces the use of 'canvas' with 'jest-canvas-mock'.
@noahtallen, @dcalhoun, thanks for your thorough reviews and guidance both. I believe I've addressed all your points, but let me know if anything more comes up!
This is a really good point. If we do find a drop-off or problems in our own testing, it'd probably be worth reverting the animations and falling back to a static icon. As Shaun is AFK this week, perhaps the next step (pending any further review on the code) could be to merge this so that the changes are available on staging. This would then make it easier to test the banners across different types of networks (as well as make it easier to test other elements, like deep linking). We could then keep an eye on any potentially negative impact and make any updates that come from design feedback in a separate PR. Does that seem like a good route? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Great work! 👏🏻
One note: I was unable to get the banners to display on a tablet (emulated with Safari or Chrome developer tools). This was also the case for the production environment, so it appears unrelated to this PR. My guess is that we only display the banners for screens below a certain width. Did you experience this?
As Shaun is AFK this week, perhaps the next step (pending any further review on the code) could be to merge this so that the changes are available on staging. This would then make it easier to test the banners across different types of networks (as well as make it easier to test other elements, like deep linking). We could then keep an eye on any potentially negative impact and make any updates that come from design feedback in a separate PR. Does that seem like a good route?
This sounds like a good plan to me. 👍🏻
Thank you!
Ah, yes, I see you're right! While testing, I'd removed the I'll go ahead to merge now. |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7507145 Thank you @SiobhyB for including a screenshot in the description! This is really helpful for our translators. |
This PR updates the app banners that are displayed in the Reader, Notifications, Stats, and editor via the mobile web. References to the WordPress app are updated to Jetpack, and the opportunity has also been taken to update the banner's design.
} | ||
} | ||
|
||
function BannerIcon( { icon } ) { | ||
useEffect( () => { | ||
const animation = lottie.loadAnimation( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just learning about Lottie and I had a question: does the library take into account @media/prefers-reduced-motion
setting out of the box or would that need extra attention here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that, no, Lottie does not take into account motion preferences.
You raise a good point. It's an opportunity to improve this new feature. I would imagine we should render the last frame of the animation when reduced motion is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! I'll work on a PR to take reduced animation preferences into account. 🙇♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To tie up all the loose ends, I've gone ahead to create that PR here: #67826
I tried a few times to get the banner to trigger, and was unsuccessful. However, since you did such an excellent job writing up this PR with a bunch of screenshots, I can still give some feedback! The only note I have is about the font-weight. I see it's been set to |
Translation for this Pull Request has now been finished. |
@shaunandrews, thank you for the feedback! @dcalhoun also spotted some differences between the banner and the initial design you provided in a separate review, so I've gone ahead to create a separate (currently draft) PR with design updates at #67607. While that PR's in draft mode, I'll aim to keep the before/after screenshots up-to-date. I've changed the title's |
Context: Following the plans to refocus the WordPress apps on core features, we plan to move features that are specific to WordPress.com to the Jetpack app. As such, we will eventually replace all mentions of the WordPress app in Calypso with Jetpack.
Proposed Changes
This PR updates the app banners that are displayed in the Reader, Notifications, Stats, and editor via the mobile web. References to the WordPress app are updated to Jetpack, and the opportunity has also been taken to update the banner's design.
Note, the banner's links won't lead to the Jetpack app with this iteration. This PR is mainly focused on updating the banner's design and the groundwork necessary for the links to work in the future.
I've given an overview of the code changes in the sections below (click each section's heading for more).
Banner UI changes⤵️
jetpack/app-branding
feature flag (added in Jetpack App: Add 'jetpack/app-branding' flag #66814) is used to add a new banner that displays when the flag is set to true (currently only for wpcalypso and development builds).jetpackAppBanner
component is similar to the currentwordpressAppBanner
component, except with changes to the copy, the inclusion of theBannerIcon
component, and the addition of thejetpack
class name for styling.jetpack
class name is currently used to specifically target UI elements in thejetpackAppBanner
, but will eventually be removed (along with other styles specific towordpressAppBanner
) when the changes are ready to be shipped to users.Icon animations⤵️
lottie-web
dependency has been added to thepackage.json
andyarn.lock
files. This dependency enables the usage of After Effects animations. As the full features of the library aren't used in our case, we're able to import the features we need fromlottie-web/build/player/lottie_light
for better performance.animations/app-promo
directory has been added to house the animated icons. These include RTL versions of the icons.getAppBannerData()
function, where they can then be retrieved for use in the banner, depending on the section of the screen the user is on.jest
doesn't handle canvas properly, which leads to an error in tests when combined withlottie-web
. To resolve this,canvas
has also been added as adevOnly
dependency.Deep linking⤵️
apps.wp.com
for iOS users. The Jetpack app won't open until we make updates to that site.Testing Instructions
Prerequisite: The app banner can be viewed via the mobile web (which can also be emulated using browser tools) when navigating directly to the Reader, Stats, Notifications, or the editor. Note, if the banner has previously been dismissed in your browser, it'll be necessary to clear your cache.
The following should be confirmed for both iOS and Android:
deep links
section above for further details). We should, however, confirm that the links to the WordPress app continue to function as before for users.It'd also be useful to verify the banner works as expected with the following:
Screenshots
LTR/RTL animation comparison⤵️
ltr.mov
rtl.mov
Different sections⤵️
Different colour schemes⤵️
Mobile/tablet comparison⤵️
Pre-merge Checklist
Related to #