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

Plan Icon: Update and use the component on My Plan page #14126

Open
4 tasks
delawski opened this issue Nov 26, 2019 · 4 comments
Open
4 tasks

Plan Icon: Update and use the component on My Plan page #14126

delawski opened this issue Nov 26, 2019 · 4 comments
Assignees
Labels
Admin Page React-powered dashboard under the Jetpack menu Plans [Pri] Low [Status] Blocked / Hold

Comments

@delawski
Copy link
Contributor

Problem

Even though currently there is a PlanIcon component in the codebase, it's used only in the Banner component.

Ideally we would use it in every time a plan icon is used, e.g. on the My Plan page (instead of using hard-coded images paths).

The problem is that those icons are different visually. E.g. here's the Personal plan icon rendered by PlanIcon:

Screenshot 2019-11-26 at 10 24 42

And here's the same Personal plan icon rendered on My Plan page:

Screenshot 2019-11-26 at 10 25 52

I guess the second icon is the most up-to-date.

Solution

  • Update PlanIcon to use the new icons for each plan
  • Add Jetpack Backup icons to PlanIcon
  • Replace the hard-coded icons on My Plan page with the PlanIcon
  • Optionally: rename the PlanIcon to ProductIcon so that it's more generic

Additional context

This has been discovered when working on adding Jetpack Backup to the My Plan page (here's the relevant PR).

@delawski delawski added Admin Page React-powered dashboard under the Jetpack menu [Pri] Low Plans labels Nov 26, 2019
@delawski delawski self-assigned this Nov 26, 2019
@stale
Copy link

stale bot commented May 24, 2020

This issue has been marked as stale. This happened because:

  • It has been inactive in the past 6 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@mattgawarecki
Copy link
Contributor

My original plan was to update PlanIcon references to use the ProductIcon component in @automattic/components, but that's looking more complicated the further I go. Essentially the bump I'm hitting is this:

Pretty much every "image" inside the Jetpack plugin right now is a static SVG, either as inline code or as files inside the repo itself. When we reference ProductIcon, however, its images are sourced from SVGs located in the component's node_modules directory. During the build process, Webpack tries to make these images accessible but is getting the path wrong.

To give a more concrete example, here's the relevant React code snippet:

<div className="dops-banner__icon-plan">
    <ProductIcon slug={ product } />
</div>

Here's the resulting markup for the path /wp-admin/admin.php?page=jetpack#/settings:

<div class="dops-banner__icon-plan">
    <img
        src="images/jetpack-premium-95eb8703805b446ffd38e9c7e9a028e1.svg"
        class="product-icon is-jetpack-premium"
        role="presentation"
        alt=""
    />
</div>

Note that the img src is relative to the current directory, which is /wp-admin, and our images aren't under that directory.

And all the images from ProductIcon are located at /wp-content/plugins/jetpack/_inc/build/images.

@mattgawarecki
Copy link
Contributor

mattgawarecki commented Jun 30, 2020

If anyone wants to take my work and run with it in the future, it's in the fix/old-plan-icons branch.

I should also note that I've tried adding a file-loader directive in the Jetpack plugin's Webpack configuration -- similar to the one in Calypso -- but to no avail. In webpack.config.js:

const fileLoader = FileConfig.loader( {
    outputPath: 'images',
    publicPath: `/wp-content/plugins/jetpack/_inc/build/images/`,
    esModules: true,
} );
sharedWebpackConfig.module.rules.push( fileLoader );

@monsieur-z
Copy link
Contributor

These 2 pending PRs should cover what has been mentioned in this issue, except the renaming to ProductIcon: #17243 and #17244

As @mattgawarecki said, the ideal situation would be to directly import ProductIcon from @automattic/components (icons were copied over from Calypso).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu Plans [Pri] Low [Status] Blocked / Hold
Projects
None yet
Development

No branches or pull requests

3 participants