mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
127 stars 41 forks source link

Clicking a link to a review triggers a page refresh #12362

Closed willdurand closed 6 years ago

willdurand commented 6 years ago

Describe the problem and steps to reproduce it:

  1. Go to the page listing the reviews of an add-on
  2. Click the relative time of one review (it has a link)

What happened?

A page refresh.

What did you expect to happen?

Client-side navigation, no page refresh.

Anything else we should know?

The problem comes from these lines: https://github.com/mozilla/addons-frontend/blob/3547c977acd733874dd2b908ae171c2091e4f496/src/amo/components/AddonReviewCard/index.js#L303-L309

We use the a HTML element and not the Link React component, by-passing the client-side navigation. We should not be doing this but I am not sure to know how to fix this.

NB: there is a similar problem with the link to a user profile from an add-on detail page.

bobsilverberg commented 6 years ago

I changed this from a Link to an <a> when I made just the timestamp the link rather than the whole byLine. Because timestamp is a string, I don't think we can just switch the <a> back to a Link. I suppose we could break it up into separate parts, but that would mess with l10n.

Like you, I'm not sure how to fix this. Perhaps we could change the text so that it always says posted [timestamp] and then I think we can semantically separate that from the by [author] part so that each of those could be a separate translated string. Then we could link the entire posted [timestamp], rather than just the timestamp and I think that would work.

What do you think, @kumar303?

kumar303 commented 6 years ago

Whoa. That's tricky. It's important that the localizers are able to position %(timestamp)s where they need to within the sentence.

We could use a placeholder, split on it, then reconstruct the string, maybe?

diff --git a/src/amo/components/AddonReviewCard/index.js b/src/amo/components/AddonReviewCard/index.js
index 8dda2e03e..48b230bbc 100644
--- a/src/amo/components/AddonReviewCard/index.js
+++ b/src/amo/components/AddonReviewCard/index.js
@@ -300,33 +300,36 @@ export class AddonReviewCardBase extends React.Component<InternalProps> {
     const noAuthor = shortByLine || this.isReply();

     if (review) {
-      const timestamp = `
-        <a href="/${lang}/${clientApp}/addon/${
+      const url = `/${lang}/${clientApp}/addon/${
         review.reviewAddon.slug
-      }/reviews/${review.id}/">
-          ${i18n.moment(review.created).fromNow()}
-        </a>
-      `;
+      }/reviews/${review.id}/`;
+      const timestamp = (
+        // Replace this with <Link/>
+        <a href={url}>{i18n.moment(review.created).fromNow()}</a>
+      );
+
       const byLineString = noAuthor
         ? // translators: Example in English: "posted last week"
           i18n.gettext('posted %(timestamp)s')
         : // translators: Example in English: "by UserName123, last week"
           i18n.gettext('by %(authorName)s, %(timestamp)s');

+      const localized = i18n.sprintf(byLineString, {
+        authorName: review.userName,
+        timestamp: '__timestamp__',
+      });
+
+      const parts = localized.split('__timestamp__');
+      const allParts = [parts.shift(), timestamp, ...parts];
+
       byLine = (
         <span
           className={makeClassName('', {
             'AddonReviewCard-authorByLine': !noAuthor,
           })}
-          // eslint-disable-next-line react/no-danger
-          dangerouslySetInnerHTML={sanitizeHTML(
-            i18n.sprintf(byLineString, {
-              authorName: review.userName,
-              timestamp,
-            }),
-            ['a'],
-          )}
-        />
+        >
+          {allParts}
+        </span>
       );
     } else {
       byLine = <LoadingText />;
muffinresearch commented 6 years ago

One way to fix this might be to use a "container" component that captures the click from the a which in turn then calls the correct api in the router. react-router-bootstrap solves this problem for users of react-bootstrap since react-bootstrap users will commonly use components that internally use a elements and it's often not possible to use the router's Link component.

I had exactly this problem when building out the projects app.

ioanarusiczki commented 6 years ago

@bobsilverberg Checked this with FF62Win10 on AMO dev Every time the time stamp of a different review is clicked the rating breakdown below the add-ons name is reloading. It's the same behavior on AMO stage too.
breakdown reloads

Let me know if it's as expected now. Thanks!

bobsilverberg commented 6 years ago

@ioanarusiczki I think that this issue has been fixed, as the page no longer refreshes, but the fact that it still reloads the add-on card on the left is something that we should try to fix. Could you open a separate issue for that, please?

ioanarusiczki commented 6 years ago

@bobsilverberg Alright, I filed https://github.com/mozilla/addons/issues/12462