Skip to content

Implement error overlay #381

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 17, 2016
Merged

Implement error overlay #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 16 additions & 1 deletion src/disco/components/Addon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { gettext as _ } from 'core/utils';
import InstallButton from 'disco/containers/InstallButton';
import {
validAddonTypes,
validInstallStates,
ERROR,
EXTENSION_TYPE,
THEME_TYPE,
THEME_PREVIEW,
Expand All @@ -18,14 +20,17 @@ import 'disco/css/Addon.scss';
export default class Addon extends React.Component {
static propTypes = {
accentcolor: PropTypes.string,
closeErrorAction: PropTypes.func,
editorialDescription: PropTypes.string.isRequired,
errorMessage: PropTypes.string,
footerURL: PropTypes.string,
headerURL: PropTypes.string,
heading: PropTypes.string.isRequired,
id: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
slug: PropTypes.string.isRequired,
subHeading: PropTypes.string,
status: PropTypes.oneOf(validInstallStates).isRequired,
textcolor: PropTypes.string,
type: PropTypes.oneOf(validAddonTypes).isRequired,
themeAction: PropTypes.func,
Expand All @@ -41,6 +46,15 @@ export default class Addon extends React.Component {
return JSON.stringify({id, name, headerURL, footerURL, textcolor, accentcolor});
}

getError() {
const { status } = this.props;
const errorMessage = this.props.errorMessage || _('An unexpected error occurred');
return status === ERROR ? (<div className="error">
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 be namespacing these classes. Maybe we should be prefixing the component name?

Addon-error, Addon-error-message, Addon-error-close?

<p className="message">{errorMessage}</p>
<a className="close" href="#" onClick={this.props.closeErrorAction}>Close</a>
</div>) : null;
}

getLogo() {
const { id } = this.props;
const imageURL = `https://addons-dev-cdn.allizom.org/user-media/addon_icons/0/${id}-64.png?modified=1388632826`;
Expand Down Expand Up @@ -96,8 +110,9 @@ export default class Addon extends React.Component {
return (
<div className={addonClasses}>
{this.getThemeImage()}
{this.getLogo()}
<div className="content">
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, classes like content are probably going to get us into trouble later.

{this.getLogo()}
{this.getError()}
<div className="copy">
<h2 ref="heading" className="heading">{heading} {subHeading ?
<span ref="sub-heading" className="sub-heading">{subHeading}</span> : null}</h2>
Expand Down
72 changes: 55 additions & 17 deletions src/disco/css/Addon.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,21 @@
$addon-padding: 20px;

.addon {
align-items: center;
background: #fff;
display: flex;
flex-direction: row;
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 this can be omitted but I'm cool with being explicit too.

line-height: 1.5;
margin-top: 20px;

&.theme {
Copy link
Contributor

Choose a reason for hiding this comment

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

And these definitely don't all need to change right now since this is a bigger issue than these changes, but I'd rather see us add more specific classes than nest our classes. Maybe not even use & or at least not using it for a class, &:before, &:hover, etc might be fine.

I'd rather see .addon--theme or I guess .Addon-theme (or .Addon--theme?) as I proposed above.

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've generally looked to use CSS as it was intended, and find that's a simple approach generally. Two classes on an element e.g. .addon.theme seems to say what it needs to say. This is an addon that's also a theme.

If a class is on the "component/widget" and the related classes for things inside it are nested that generally works well. I've not used any of the naming schemes as you describe - I've not yet seen seen a scenario where they are necessary especially if the markup is right. That said we should not let historical usage prevent us from considering fresh approaches to CSS (this is something we can definitely talk about in London) and if we have a consensus that the something like BEM makes sense we could adopt it. But I feel I'd want to see that it really adds value before we jump to using something like that - my gut reaction is that it's overkill.

As long as nesting doesn't go too deep then for this project I'm fine with what we're doing - we could probably lint for that. What can be a problem with the likes of SASS and like any abstraction is that you can do bad things un-intentionally. E.g. you can over-nest and yet the src looks fine but the output is heavily over-specified. Again I think this is something we could use a CSS linter for to guard against and watch out for.

display: block;

.content .copy {
padding: 20px;
}
}

.theme-image {
display: block;

Expand All @@ -24,23 +36,52 @@ $addon-padding: 20px;
margin: 0 20px 0 30px;
}

.content {
.logo {
align-items: center;
align-self: stretch;
display: flex;
background: #fcfcfc;
padding: 0 15px;

img {
display: block;
height: 64px;
width: 64px;
}
}

.content {
display: flex;
align-items: center;
flex-direction: row;
line-height: 1.5;
flex-grow: 1;
position: relative;

.logo {
.error {
align-content: stretch;
align-items: center;
align-self: stretch;
background: #fcfcfc;
background: #c33c32 url('../img/warning.svg') no-repeat calc(100% - 40px) 50%;
bottom: 0;
color: #fff;
display: flex;
padding: 0 15px;
flex-direction: row;
left: 0;
padding: 15px 20px;
position: absolute;
right: 0;
top: 0;
z-index: 10;

img {
.message {
flex-grow: 1;
}

.close {
color: #fff;
display: block;
height: 64px;
width: 64px;
position: absolute;
left: 20px;
bottom: 15px;
}
}

Expand All @@ -53,12 +94,12 @@ $addon-padding: 20px;
font-size: 18px;
font-weight: medium;
margin: 0;
}

.sub-heading {
color: $secondary-font-color;
font-size: 14px;
font-weight: normal;
}
.sub-heading {
color: $secondary-font-color;
font-size: 14px;
font-weight: normal;
}

.editorial-description {
Expand All @@ -74,7 +115,4 @@ $addon-padding: 20px;
}
}

&.theme .content .copy {
padding: 20px;
}
}
5 changes: 5 additions & 0 deletions src/disco/img/warning.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 16 additions & 1 deletion tests/client/disco/components/TestAddon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { Provider } from 'react-redux';
import { createStore } from 'redux';
import Addon from 'disco/components/Addon';

import { THEME_PREVIEW, THEME_RESET_PREVIEW } from 'disco/constants';
import { ERROR, THEME_PREVIEW, THEME_RESET_PREVIEW } from 'disco/constants';


const result = {
Expand Down Expand Up @@ -38,6 +38,21 @@ describe('<Addon type="Extension"/>', () => {
root = renderAddon(result);
});

it('renders an error overlay', () => {
const data = {...result, status: ERROR,
errorMessage: 'this is an error', closeErrorAction: sinon.stub()};
root = renderAddon(data);
const error = findDOMNode(root).querySelector('.error');
assert.equal(error.querySelector('p').textContent, 'this is an error',
'error message should be present');
Simulate.click(error.querySelector('.close'));
assert.ok(data.closeErrorAction.called, 'close link action should be called');
});

it('does not normally render an error', () => {
assert.notOk(findDOMNode(root).querySelector('.error'));
});

it('renders the heading', () => {
assert.include(root.refs.heading.textContent, 'test-heading');
});
Expand Down