Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
a9d26d7
Handle case when add-on hasn't been loaded yet
kumar303 Jun 14, 2017
a92b898
Merge branch 'master' into addon-saga-iss2442
kumar303 Jun 14, 2017
c0e2e14
Make sure addon.type doesn't get dispatched
kumar303 Jun 14, 2017
adb03c9
Happy path of fetching an add-on works via saga
kumar303 Jun 15, 2017
0db59f3
Merge branch 'master' into addon-saga-iss2442
kumar303 Jun 16, 2017
3c9aa84
Merge branch 'master' into addon-saga-iss2442
kumar303 Jun 16, 2017
5bbe60a
handle errors and also fix displaying 502 proxy errors
kumar303 Jun 16, 2017
91a8fb6
move the add-ons saga to core
kumar303 Jun 16, 2017
e413efc
Merge branch 'master' into addon-saga-iss2442
kumar303 Jun 16, 2017
577a751
fix merge fallout
kumar303 Jun 16, 2017
dca6cec
Handle add-on 404s
kumar303 Jun 19, 2017
7ba50a1
Merge branch 'master' into addon-saga-iss2442
kumar303 Jun 20, 2017
da0d1fa
Synced AddonDetail HTML changes with master
kumar303 Jun 20, 2017
bf94cb8
remove no-danger lint comment
kumar303 Jun 20, 2017
c47ae07
Use .children, duh
kumar303 Jun 20, 2017
845e67d
Add a fixedWidth prop to LoadingText
kumar303 Jun 20, 2017
1fc454b
Rename fixedWidth prop to width
kumar303 Jun 20, 2017
97d7676
Use fixed width placeholders
kumar303 Jun 20, 2017
4663cad
Converted TestAddonMeta to use shallow(), fixed lint
kumar303 Jun 20, 2017
f74bda5
Let AddonMeta render with a null addon
kumar303 Jun 20, 2017
72d3ce7
Clean up some TODOs
kumar303 Jun 20, 2017
dcf1537
update view context when props get updated
kumar303 Jun 20, 2017
03e5277
Use shorthand for `store`
kumar303 Jun 20, 2017
6865526
Only set view context on update when necessary
kumar303 Jun 20, 2017
ae14938
Simpler code to check for addon type changes
kumar303 Jun 21, 2017
2dd84c7
Add missing tests for error handler action creators
kumar303 Jun 21, 2017
7273f81
Merge branch 'master' into addon-saga-iss2442
kumar303 Jun 23, 2017
95f0e82
Change 'No reviews yet' to LoadingText
kumar303 Jun 23, 2017
da92132
false -> null
kumar303 Jun 23, 2017
c4194a9
use fixed width for user count placeholder
kumar303 Jun 23, 2017
1069c03
Convert most of TestAddon to Enzyme
kumar303 Jun 23, 2017
59db61f
fix abbreviation
kumar303 Jun 23, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
224 changes: 144 additions & 80 deletions src/amo/components/Addon/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* eslint-disable react/no-danger */
/* eslint-disable jsx-a11y/heading-has-content */
import classNames from 'classnames';
import React from 'react';
import PropTypes from 'prop-types';
Expand All @@ -9,25 +9,30 @@ import { setViewContext } from 'amo/actions/viewContext';
import AddonCompatibilityError from 'amo/components/AddonCompatibilityError';
import AddonMeta from 'amo/components/AddonMeta';
import AddonMoreInfo from 'amo/components/AddonMoreInfo';
import NotFound from 'amo/components/ErrorPage/NotFound';
import DefaultRatingManager from 'amo/components/RatingManager';
import ScreenShots from 'amo/components/ScreenShots';
import Link from 'amo/components/Link';
import fallbackIcon from 'amo/img/icons/default-64.png';
import { fetchAddon } from 'core/actions/addons';
import { withErrorHandler } from 'core/errorHandler';
import InstallButton from 'core/components/InstallButton';
import { ADDON_TYPE_THEME, ENABLED, UNKNOWN } from 'core/constants';
import {
ADDON_TYPE_EXTENSION, ADDON_TYPE_THEME, ENABLED, UNKNOWN,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit but once we start doing more imports than fit on a line it's one-per-line eg:

import {
  ADDON_TYPE_EXTENSION,
  ADDON_TYPE_THEME,
  ENABLED,
  UNKNOWN,
} from 'core/constants';

Though I really wish we could have a rule for that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit but once we start doing more imports than fit on a line it's one-per-line

it's pretty easy to fix once they can't fit on a line anymore

} from 'core/constants';
import { withInstallHelpers } from 'core/installAddon';
import {
isAllowedOrigin,
getClientCompatibility as _getClientCompatibility,
loadAddonIfNeeded,
nl2br,
safeAsyncConnect,
sanitizeHTML,
} from 'core/utils';
import translate from 'core/i18n/translate';
import log from 'core/logger';
import Button from 'ui/components/Button';
import Card from 'ui/components/Card';
import Icon from 'ui/components/Icon';
import LoadingText from 'ui/components/LoadingText';
import ShowMoreCard from 'ui/components/ShowMoreCard';

import './styles.scss';
Expand Down Expand Up @@ -55,11 +60,13 @@ export class AddonBase extends React.Component {
addon: PropTypes.object.isRequired,
clientApp: PropTypes.string.isRequired,
dispatch: PropTypes.func.isRequired,
errorHandler: PropTypes.object.isRequired,
getClientCompatibility: PropTypes.func,
getBrowserThemeData: PropTypes.func.isRequired,
i18n: PropTypes.object.isRequired,
isPreviewingTheme: PropTypes.bool.isRequired,
location: PropTypes.object.isRequired,
params: PropTypes.object.isRequired,
resetThemePreview: PropTypes.func.isRequired,
themePreviewNode: PropTypes.element,
installStatus: PropTypes.string.isRequired,
Expand All @@ -73,9 +80,21 @@ export class AddonBase extends React.Component {
}

componentWillMount() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice there's nothing here for componentDidUpdate–if an add-on linked to another add-on the components on the page wouldn't change but the props would update and we would want to do things like update the viewContext.

Of course, it would be nice if we did that more often with routing–as in this case we could get away with using the URL to know addonType but for now we should dispatch(setViewContext()) on componentDidUpdate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed now

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks; sorry it was an awkward place to leave a comment as I know it won't be cleared. 😅

const { addon, dispatch } = this.props;
const { addon, dispatch, errorHandler, params } = this.props;

if (addon) {
dispatch(setViewContext(addon.type));
} else {
dispatch(fetchAddon({ slug: params.slug, errorHandler }));
}
}

dispatch(setViewContext(addon.type));
componentWillReceiveProps({ addon: newAddon }) {
const { addon: oldAddon, dispatch } = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

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

Woooah I've never seen this syntax! I'm guessing it's the same as:

const addon = this.props.oldAddon;
const dispatch = this.props.dispatch;

Neat-o, just didn't know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's the greatest syntax. It's actually a shortcut for this which is different from the code you posted:

const oldAddon = this.props.addon;
const dispatch = this.props.dispatch;

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh, wow. That's not what I expect, making it a bit hard to read. Maybe it would be nicer to do:

const { dispatch } = this.props;
const addon = this.props.oldAddon;

I just find that syntax opposite to what I expect.

Copy link
Contributor Author

@kumar303 kumar303 Jun 23, 2017

Choose a reason for hiding this comment

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

I don't think we should add workarounds for standard JavaScript syntax. It is what it is and it's well documented. It will be around for a long time :) We can help teach contributors if they get stuck with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plus, your workaround suggestion adds an extra line of code -- less code is always easier to read IMO.

Copy link
Contributor

@tofumatt tofumatt Jun 23, 2017

Choose a reason for hiding this comment

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

Fair enough!

I don't think it's easier to read, but it is a standard syntax so at least someone can look it up. I think there are language features we don't use because they aren't pleasant and I'd nominate this one as well... I think it's easier to reason about what this is doing with the two lines of code, and it's not logic just assignment so I think it doesn't add complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once I got used to it it no longer seemed foreign. I really wish it looked more like this but oh well:

const { addon as oldAddon } = this.props;

^ This is how I read it in my head now which helps me parse it.

Copy link
Contributor

Choose a reason for hiding this comment

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

omg, agreed. Why didn't they just do that?!

That actually helps though, thanks 👍

const oldAddonType = oldAddon ? oldAddon.type : null;
if (newAddon && newAddon.type !== oldAddonType) {
dispatch(setViewContext(newAddon.type));
}
}

componentWillUnmount() {
Expand All @@ -102,8 +121,9 @@ export class AddonBase extends React.Component {
isPreviewingTheme,
installStatus,
} = this.props;
const { previewURL, type } = addon;
const iconUrl = isAllowedOrigin(addon.icon_url) ? addon.icon_url :
const previewURL = addon ? addon.previewURL : null;
const type = addon ? addon.type : ADDON_TYPE_EXTENSION;
const iconUrl = addon && isAllowedOrigin(addon.icon_url) ? addon.icon_url :
fallbackIcon;

if (type === ADDON_TYPE_THEME) {
Expand Down Expand Up @@ -145,7 +165,7 @@ export class AddonBase extends React.Component {
let content;
let footerPropName;

if (addon.ratings.count) {
if (addon && addon.ratings.count) {
const count = addon.ratings.count;
const linkText = i18n.sprintf(
i18n.ngettext('Read %(count)s review', 'Read all %(count)s reviews', count),
Expand Down Expand Up @@ -173,69 +193,127 @@ export class AddonBase extends React.Component {
header={i18n.gettext('Rate your experience')}
className="Addon-overall-rating"
{...props}>
<RatingManager
addon={addon}
location={location}
version={addon.current_version}
/>
{addon ?
<RatingManager
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 it should be done in another patch, I guess the Reviews one maybe but it would be great if this showed LoadingText in place of "No reviews yet" as well. Anyway that's just a note/UX thing to keep in mind, this patch is big enough now and no need to introduce more complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, yeah, I was thinking about this too. The 'No reviews yet' is an answer to a question asked, which is 'how many reviews are there for this add-on?' In this case, it is not the right answer to give. I'll see if I can change it without much churn.

Copy link
Contributor

Choose a reason for hiding this comment

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

I commented below–it looks like it should be straightforward.

But if it becomes a beast I'm happy to wait for another patch.

addon={addon}
location={location}
version={addon.current_version}
/> : null
}
</Card>
);
}

renderShowMoreCard() {
const { addon, i18n } = this.props;
const addonType = addon ? addon.type : ADDON_TYPE_EXTENSION;
let description;

const descriptionProps = {};
if (addon) {
description = addon.description ? addon.description : addon.summary;
if (!description || !description.length) {
return null;
}
descriptionProps.dangerouslySetInnerHTML = sanitizeHTML(
nl2br(description), allowedDescriptionTags);
} else {
descriptionProps.children = <LoadingText width={100} />;
}

return (
<ShowMoreCard header={i18n.sprintf(
i18n.gettext('About this %(addonType)s'), { addonType }
)} className="AddonDescription">
<div className="AddonDescription-contents"
ref={(ref) => { this.addonDescription = ref; }}
{...descriptionProps}
/>
</ShowMoreCard>
);
}

render() {
const {
addon,
clientApp,
errorHandler,
getClientCompatibility,
i18n,
installStatus,
userAgentInfo,
} = this.props;

const authorList = addon.authors.map(
(author) => `<a href="${author.url}">${author.name}</a>`);
const description = addon.description ? addon.description : addon.summary;
// Themes lack a summary so we do the inverse :-/
// TODO: We should file an API bug about this...
const summary = addon.summary ? addon.summary : addon.description;

const title = i18n.sprintf(
// L10n: Example: The Add-On <span>by The Author</span>
i18n.gettext('%(addonName)s %(startSpan)sby %(authorList)s%(endSpan)s'), {
addonName: addon.name,
authorList: authorList.join(', '),
startSpan: '<span class="Addon-author">',
endSpan: '</span>',
});
let errorBanner = null;
if (errorHandler.hasError()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen this HOC used much but that's really nice in usage. 👍

log.error('Captured API Error:', errorHandler.capturedError);
if (errorHandler.capturedError.responseStatusCode === 404) {
return <NotFound />;
}
// Show a list of errors at the top of the add-on section.
errorBanner = errorHandler.renderError();
}

const {
compatible, maxVersion, minVersion, reason,
} = getClientCompatibility({ addon, clientApp, userAgentInfo });
const addonType = addon ? addon.type : ADDON_TYPE_EXTENSION;

const summaryProps = {};
if (addon) {
// Themes lack a summary so we do the inverse :-/
// TODO: We should file an API bug about this...
const summary = addon.summary ? addon.summary : addon.description;
summaryProps.dangerouslySetInnerHTML = sanitizeHTML(summary, ['a']);
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 should remove the /* eslint-disable react/no-danger */ at the top of the file and around the render() method–instead we should just use // eslint-disable-next-line react/no-danger, as it's a good thing to call out and recognise every time we use it 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, we don't even need it because the linter isn't smart enough to follow the summaryProps object 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no! 😆

} else {
summaryProps.children = <LoadingText width={100} />;
}

const titleProps = {};
if (addon) {
const authorList = addon.authors.map(
(author) => `<a href="${author.url}">${author.name}</a>`);
const title = i18n.sprintf(
// L10n: Example: The Add-On <span>by The Author</span>
i18n.gettext('%(addonName)s %(startSpan)sby %(authorList)s%(endSpan)s'), {
addonName: addon.name,
authorList: authorList.join(', '),
startSpan: '<span class="Addon-author">',
endSpan: '</span>',
}
);
titleProps.dangerouslySetInnerHTML = sanitizeHTML(title, ['a', 'span']);
} else {
titleProps.children = <LoadingText width={70} />;
}

const addonPreviews = addon ? addon.previews : [];

let isCompatible = false;
let compatibility;
if (addon) {
compatibility = getClientCompatibility({
addon, clientApp, userAgentInfo,
});
isCompatible = compatibility.compatible;
}

// eslint-disable react/no-danger
return (
<div className={classNames('Addon', `Addon-${addon.type}`)}>
<div className={classNames('Addon', `Addon-${addonType}`)}>
{errorBanner}
<Card className="" photonStyle>
<header className="Addon-header">
<h1
className="Addon-title"
dangerouslySetInnerHTML={sanitizeHTML(title, ['a', 'span'])}
/>
<h1 className="Addon-title" {...titleProps} />
<p className="Addon-summary" {...summaryProps} />

<p
className="Addon-summary"
dangerouslySetInnerHTML={sanitizeHTML(summary, ['a'])}
/>
{addon ?
<InstallButton
{...this.props}
className="Button--action Button--small"
disabled={!isCompatible}
ref={(ref) => { this.installButton = ref; }}
status={installStatus}
/> : null
}

<InstallButton
{...this.props}
className="Button--action Button--small"
disabled={!compatible}
ref={(ref) => { this.installButton = ref; }}
status={installStatus}
/>

{this.headerImage({ compatible })}
{this.headerImage({ compatible: isCompatible })}

<h2 className="visually-hidden">
{i18n.gettext('Extension Metadata')}
Expand All @@ -244,43 +322,29 @@ export class AddonBase extends React.Component {
<AddonMeta addon={addon} />
</header>

{!compatible ? (
<AddonCompatibilityError maxVersion={maxVersion}
minVersion={minVersion} reason={reason} />
{compatibility && !isCompatible ? (
<AddonCompatibilityError
maxVersion={compatibility.maxVersion}
minVersion={compatibility.minVersion}
reason={compatibility.reason}
/>
) : null}
</Card>

<div className="Addon-details">
{addon.previews.length > 0 ? (
{addonPreviews.length > 0 ? (
<Card
className="Addon-screenshots"
header={i18n.gettext('Screenshots')}
>
<ScreenShots previews={addon.previews} />
<ScreenShots previews={addonPreviews} />
</Card>
) : null}

{description && description.length ? (
<ShowMoreCard
header={i18n.sprintf(
i18n.gettext('About this %(addonType)s'),
{ addonType: addon.type }
)}
className="AddonDescription"
>
<div
className="AddonDescription-contents"
ref={(ref) => { this.addonDescription = ref; }}
dangerouslySetInnerHTML={
sanitizeHTML(nl2br(description), allowedDescriptionTags)
}
/>
</ShowMoreCard>
) : null}

{this.renderShowMoreCard()}
{this.renderRatingsCard()}

<AddonMoreInfo addon={addon} />
{addon ? <AddonMoreInfo addon={addon} /> : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here as for AddonMeta; handling an empty addon would be good and we should render this component with the LoadingText. That way fewer elements appear out of nowhere once the add-on loads and the page looks less empty whilst loading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AddonMoreInfo was pretty tough to convert. I don't think it's worth the effort right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to file an issue then. I find it weird that it says "no reviews" while it's loading. The empty review stars are fine (that's actually what I'd stick with) but the "no reviews" instead of <LoadingText /> isn't the best UX:

jun-22-2017 21-46-20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 'No reviews yet' is fixed.

As for AddonMoreInfo, I don't think it's worth spending time on but, sure, I filed and marked it as contrib: welcome https://github.com/mozilla/addons-frontend/issues/2629

</div>
</div>
);
Expand All @@ -291,7 +355,10 @@ export class AddonBase extends React.Component {
export function mapStateToProps(state, ownProps) {
const { slug } = ownProps.params;
const addon = state.addons[slug];
const installedAddon = state.installations[addon.guid] || {};
let installedAddon = {};
if (addon) {
installedAddon = state.installations[addon.guid] || {};
}

return {
addon,
Expand All @@ -315,11 +382,8 @@ export function mapStateToProps(state, ownProps) {
}

export default compose(
safeAsyncConnect([{
key: 'Addon',
promise: loadAddonIfNeeded,
}]),
translate({ withRef: true }),
connect(mapStateToProps),
withInstallHelpers({ src: 'dp-btn-primary' }),
withErrorHandler({ name: 'Addon' }),
)(AddonBase);
Loading