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

Add dashboard wincher connect #20107

Merged

Conversation

KaisZaoualiWincher
Copy link
Contributor

@KaisZaoualiWincher KaisZaoualiWincher commented Apr 3, 2023

Context

Display the Wincher performance report component on the dashboard even for not logged-in users, and add a reconnect button and fake table data.

Summary

This PR can be summarized in the following changelog entry:

  • Display the Wincher performance report component for not logged-in users, on the dashboard.
  • Add connect button.
  • Add fake table data.
  • Add table description.

Test instructions

  • Go to the dashboard.
  • Check "Top performing keyphrases on your site" section (i.e. Wincher performance component)

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

  • With a not connected user, the Wincher performance component should be displayed.
  • Connect button should invoke login action

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Block/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

  • Wincher performance report component should be able to display data without regression for logged-in users.
  • Edge cases (e.g. unauthorized to fetch table data, empty data, etc) handling should work without regression.

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities.
  • During testing, I had activated all plugins Yoast SEO provides integrations for.
  • I have added unit tests to verify the code works as intended.
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label and noted the work hours.

Fixes #

-moz-filter: blur(2px);
-o-filter: blur(2px);
-ms-filter: blur(2px);
filter: blur(2px);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should have a bit more blur here. Like 4-5px

{ isLoggedIn && data && ! isEmpty( data ) && ! isEmpty( data.results ) && <Fragment>
<TableExplanation />
{ data && ! isEmpty( data ) && ! isEmpty( data.results ) && <Fragment>
<TableExplanation isLoggedIn={ isLoggedIn } />

<WincherSEOPerformanceTableWrapper>
<table className="yoast yoast-table">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we ought to disable pointer events and user select on the table (not wrapper) if it's blurred:

pointer-events: none;
user-select: none;

@KaisZaoualiWincher KaisZaoualiWincher force-pushed the feature/add-dashboard-wincher-connect branch from b4ee40d to 4514836 Compare April 13, 2023 02:37
@oksanayoast
Copy link

oksanayoast commented Apr 13, 2023

Hi @KaisZaoualiWincher I have several questions:

  1. Could you please provide screenshots for each user case you are mentioning in the description:
    - when the user is logged out from Wincher and fake data is displayed
    - you mentioned the "reconnect button": is it a separate case?
  2. Should we display a success/error message after the connection is performed?
    Where should it be displayed? Screenshots would also help here.
    Example in post editor screen: You have successfully connected to Wincher! You can now track the SEO performance for the keyphrase(s) of this page.
  3. After the successful connection - will the whole page reload to get the new data, or just the widget or its section will be refreshed via AJAX?
  4. What happens when the Wincher integration is switched off?

@KaisZaoualiWincher
Copy link
Contributor Author

KaisZaoualiWincher commented Apr 13, 2023

Hi @KaisZaoualiWincher I have several questions:

  1. Could you please provide screenshots for each user case you are mentioning in the description:
    • when the user is logged out from Wincher and fake data is displayed
    • you mentioned the "reconnect button": is it a separate case?
  2. Should we display a success/error message after the connection is performed?
    Where should it be displayed? Screenshots would also help here.
    Example in post editor screen: You have successfully connected to Wincher! You can now track the SEO performance for the keyphrase(s) of this page.
  3. After the successful connection - will the whole page reload to get the new data, or just the widget or its section will be refreshed via AJAX?
  4. What happens when the Wincher integration is switched off?

Hi @oksanayoast!

  1. No, it's the same use case i.e. when a user is logged out, now we display a blurred table with fake data, and a "connect with Wincher" button. Previously we were not displaying the Wincher SEO performance report on the dashboard page if the user is logged out.

    • Before:

    image

    • After:

    image

  2. We need to have a custom success alert. I will add it.

  3. No reload, the component is already mounted.

  4. The Wincher SEO performance report component will not be displayed. This has not been impacted by the current PR.

@KaisZaoualiWincher
Copy link
Contributor Author

Two success alerts are now added to the Wincher connect action:

  1. Successful connect + user already tracking keyphrases:

image

  1. Successful connect + user has not tracked keyphrases yet:

image

@leonidasmi
Copy link
Contributor

leonidasmi commented Apr 19, 2023

  • Use a shortlink instead, for the Get More Insights... link
  • A new shortlink for the Connect with Wincher to get started link
  • Do we want to link to our integration card, so that we prompt users to deactivate it if they want?
  • Check with UX, in general and with Oksana about the above.

@pls78 pls78 self-assigned this Apr 27, 2023
@pls78
Copy link
Member

pls78 commented Apr 27, 2023

Hey @KaisZaoualiWincher 👋 thanks for addressing the first two items on the list above, we forgot to specify that it was meant as a reminder for us! 🙂

@BobBreukhoven
Copy link
Contributor

@KaisZaoualiWincher can you come up with a suggestion, on how we can show the user, that they can toggle the Wincher integration "off", in our "integration settings". We should tell the users where they can deactivate it. Something like this

image

CC: @uxkai @manuelaugustin
FYI: @jonoalderson

@jonoalderson
Copy link

But the integration isn't active at this point, right?
And, nobody looking at this will have any idea what a Wincher is 😆

@uxkai
Copy link
Contributor

uxkai commented Apr 28, 2023

@jonoalderson As I understand it, the Wincher integration is active (enabled on the 'Integrations' page), but the user is not currently logged in or connected to Wincher.

@jonoalderson
Copy link

Ah, catching up; all good! :)

@KaisZaoualiWincher KaisZaoualiWincher force-pushed the feature/add-dashboard-wincher-connect branch from d2d7936 to 5361f37 Compare May 1, 2023 14:06
@KaisZaoualiWincher
Copy link
Contributor Author

The Wincher performance report is moved to its own widget on the dashboard:

image

  • The Wincher performance report widget can be disabled via the screen options separately from the Yoast widget.
  • Integration can be disabled from the integrations page.
  • Unnecessary Wincher integration code has been removed from the Yoast widget

@pls78
Copy link
Member

pls78 commented May 2, 2023

Hey @KaisZaoualiWincher 👋 in the error management could you please also take into account the case the user goes offline? For example, if I now turn the network off and click the Connect to Wincher button I get an uncaught error ( WincherDashboardWidget::onConnect method tries to build the url anyway) 🙏

@KaisZaoualiWincher
Copy link
Contributor Author

Hey @KaisZaoualiWincher 👋 in the error management could you please also take into account the case the user goes offline? For example, if I now turn the network off and click the Connect to Wincher button I get an uncaught error ( WincherDashboardWidget::onConnect method tries to build the url anyway) 🙏

Hi @pls78 👋
For the network disconnection issue related to onConnect shall we display an alert, or a notification, or only handle the exception?

@BobBreukhoven
Copy link
Contributor

@KaisZaoualiWincher please see our "Storybook": https://ui-library.yoast.com/?path=/docs/1-elements-alert--variants#variants

There we have different variants of "Alerts" that we use in our App, and if it is an error, we use the "error alert".

CC: @uxkai @pls78

@manuelaugustin manuelaugustin mentioned this pull request May 3, 2023
16 tasks
@pls78
Copy link
Member

pls78 commented May 5, 2023

Hello @KaisZaoualiWincher! As @BobBreukhoven said, we take care of error states by catching them and showing an alert 👍

@KaisZaoualiWincher
Copy link
Contributor Author

Hi @BobBreukhoven and @pls78! Thank you for the link. I will add an error alert to handle the network disconnection error.

@KaisZaoualiWincher
Copy link
Contributor Author

Hey @KaisZaoualiWincher 👋 in the error management could you please also take into account the case the user goes offline? For example, if I now turn the network off and click the Connect to Wincher button I get an uncaught error ( WincherDashboardWidget::onConnect method tries to build the url anyway) 🙏

Hi @pls78!

Network errors should be handled now:

image

@uxkai
Copy link
Contributor

uxkai commented May 9, 2023

Hi @KaisZaoualiWincher, would it be possible to decrease the margin below the error alert slightly, so it matches with the margin below "Connect with Wincher to get started."? 😄

@KaisZaoualiWincher
Copy link
Contributor Author

Hi @KaisZaoualiWincher, would it be possible to decrease the margin below the error alert slightly, so it matches with the margin below "Connect with Wincher to get started."? 😄

Should be fixed, for all the user message alerts inside the Wincher performance report in the dashboard.

image

@vraja-pro vraja-pro assigned vraja-pro and unassigned pls78 May 15, 2023
@vraja-pro vraja-pro removed their assignment May 15, 2023
@vraja-pro
Copy link
Contributor

I'm getting errors in the console when I'm on Dashboard with no connection:
Screenshot 2023-05-15 at 16 20 26

};

WincherSEOPerformanceTable.propTypes = {
isBlurred: PropTypes.object,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
isBlurred: PropTypes.object,
isBlurred: PropTypes.bool,


Cell.propTypes = {
isBlurred: PropTypes.bool,
children: PropTypes.object,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
children: PropTypes.object,
children: PropTypes.oneOfType([
PropTypes.string,
PropTypes.number,
PropTypes.object
]),

@pls78
Copy link
Member

pls78 commented May 16, 2023

Hi @KaisZaoualiWincher 👋 Can you take a look at the errors in the screenshot provided by @vraja-pro ? 🙏

@KaisZaouali
Copy link

Hi @KaisZaoualiWincher 👋 Can you take a look at the errors in the screenshot provided by @vraja-pro ? 🙏

Hi @pls78 👋 Errors should be fixed.

Hi @vraja-pro 👋 Thank you for the good catches. It's strange that no error is thrown for me, and I couldn't even reproduce it. Anyway, it should be fixed by now.

@vraja-pro vraja-pro merged commit ac2f096 into Yoast:trunk May 17, 2023
@leonidasmi leonidasmi added this to the 20.9 milestone May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet