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

Display user feedback when IPFS fails to provide annotations #3396

Merged
merged 7 commits into from
May 17, 2022
Merged
Changes from 4 commits
Commits
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
34 changes: 34 additions & 0 deletions src/modules/core/components/CalloutCard/CalloutCard.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
.main {
padding: 20px 14px 20px 14px;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just do padding: 20px 14px

width: 100%;
position: relative;
border-radius: var(--radius-normal);
background-color: var(--colony-white);
font-size: var(--size-smallish);
font-weight: var(--weight-bold);
}

/* Theme */
.themePrimary {
composes: main;
background-color: var(--primary);
color: var(--colony-white);
}

.themeInfo {
composes: main;
background-color: newInfoBlue;
color: var(--colony-white);
}

.themeDanger {
composes: main;
background-color: var(--danger);
color: var(--colony-white);
}

.themePinky {
composes: main;
background-color: color-mod(var(--pink) alpha(15%));
color: var(--dark);
}
5 changes: 5 additions & 0 deletions src/modules/core/components/CalloutCard/CalloutCard.css.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const main: string;
export const themePrimary: string;
export const themeInfo: string;
export const themeDanger: string;
export const themePinky: string;
61 changes: 61 additions & 0 deletions src/modules/core/components/CalloutCard/CalloutCard.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
**Description**
Copy link
Member

Choose a reason for hiding this comment

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

Very cool of you to add a styleguide entry for this, but I think we're going to discontinue this. (unless you guys have other opinions on this)

Leave it in for now

The CalloutCard component provides a standardsed way to display user information such as warnings.
The CalloutCard's appearance can be customized through `theme`s. The content should be provided through the appropriate props.

### Example

Here's a super simple implementation of an CalloutCard, and different examples of customisation of the content:

```jsx
import CalloutCard from '~core/CalloutCard';
import Link from '~core/Link';
import ExternalLink from '~core/ExternalLink';

const MSG = defineMessages({
warningTitle: {
id: 'dashboard.ActionsPageFeed.ActionsPageFeedItemWithIPFS.warningTitle',
defaultMessage: `<span>Attention.</span> `,
},
warningTextExternal: {
id: 'dashboard.ActionsPageFeed.ActionsPageFeedItemWithIPFS.warningText',
defaultMessage: `Unable to connect to IPFS gateway, annotations not loaded. <a>Reload to try again</a>`,
},
warningTextInternal: {
id: 'dashboard.ActionsPageFeed.ActionsPageFeedItemWithIPFS.internalLink',
defaultMessage: `Unable to connect to IPFS gateway, annotations not loaded. {link}`,
},
});

<CalloutCard
label={...MSG.warningTitle}
labelValues={{
span: (chunks) => (
<span className={styles.noIPFSLabel}>{chunks}</span>
),
}}
description={...MSG.warningTextInternal}
descriptionValues={{
link: (
<Link
to={location.pathname}
text={MSG.internalLink}
className={styles.link}
onClick={() => window.location.reload(false)}
/>
),
}}
/>

<CalloutCard
label={...MSG.warningTitle}
labelValues={{
span: (chunks) => (
<span className={styles.noIPFSLabel}>{chunks}</span>
),
}}
description={...MSG.warningTextExternal}
descriptionValues={{
a: (chunks) => <ExternalLink href="/url">{chunks}</ExternalLink>,
}}
/>
```
49 changes: 49 additions & 0 deletions src/modules/core/components/CalloutCard/CalloutCard.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import React from 'react';
import { FormattedMessage, MessageDescriptor } from 'react-intl';

import { ComplexMessageValues } from '~types/index';

import { getMainClasses } from '~utils/css';
import styles from './CalloutCard.css';

const displayName = 'CalloutCard';

interface Appearance {
theme?: 'primary' | 'info' | 'danger' | 'pinky';
}

interface Props {
/** Appearance object */
appearance?: Appearance;

/** label text */
label: MessageDescriptor | string;

/** Values for context text (react-intl interpolation) */
labelValues?: ComplexMessageValues;

/** A string or a `messageDescriptor` that make up the cards's content */
description?: MessageDescriptor | string;

/** Values for context text (react-intl interpolation) */
descriptionValues?: ComplexMessageValues;
}

const CalloutCard = ({
appearance = { theme: 'pinky' },
label,
labelValues,
description,
descriptionValues,
}: Props) => {
return (
<div className={getMainClasses(appearance, styles)}>
<FormattedMessage {...label} values={labelValues} />
<FormattedMessage {...description} values={descriptionValues} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Both for label and for description your types give string as an option and you can't destructure a string. You either need to change types to restrict it to only MessageDescriptor, or you call it depending on what type the prop is (similar to some other core components)

Copy link
Member

Choose a reason for hiding this comment

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

I think you should make them a MessageDescriptor required type. I'm not seeing this being used with simple strings anyway

</div>
);
};

CalloutCard.displayName = displayName;

export default CalloutCard;
1 change: 1 addition & 0 deletions src/modules/core/components/CalloutCard/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './CalloutCard';
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
.link {
font-weight: var(--weight-bold);
color: var(--sky-blue);
}

.link:hover {
text-decoration: underline;
}

.noIPFSLabel {
font-size: var(--size-smallish);
font-weight: var(--weight-bold);
color: var(--pink);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const link: string;
export const noIPFSLabel: string;
export const noIpfsLabel: string;
Original file line number Diff line number Diff line change
@@ -1,12 +1,32 @@
import React, { useCallback } from 'react';
import { useLocation } from 'react-router-dom';
import { defineMessages } from 'react-intl';

import Comment, { Props as CommentProps } from '~core/Comment';
import CalloutCard from '~core/CalloutCard';
import Link from '~core/Link';

import { useDataFetcher } from '~utils/hooks';
import { ipfsDataFetcher } from '../../../../core/fetchers';

import styles from './ActionsPageFeedItemWithIPFS.css';

const displayName = 'dashboard.ActionsPageFeed.ActionsPageFeedItemWithIPFS';

const MSG = defineMessages({
warningTitle: {
id: 'dashboard.ActionsPageFeed.ActionsPageFeedItemWithIPFS.warningTitle',
defaultMessage: `<span>Attention.</span> `,
},
warningText: {
id: 'dashboard.ActionsPageFeed.ActionsPageFeedItemWithIPFS.internalLink',
defaultMessage: `Unable to connect to IPFS gateway, annotations not loaded. {link}`,
},
reloadLink: {
id: 'dashboard.ActionsPageFeed.ActionsPageFeedItemWithIPFS.internalLink',
defaultMessage: `Reload to try again`,
},
});
interface Props extends CommentProps {
hash: string;
}
@@ -24,7 +44,7 @@ const ActionsPageFeedItemWithIPFS = ({
);

const getAnnotationMessage = useCallback(() => {
if (!annotation || !ipfsDataJSON) {
if (!annotation) {
return undefined;
}
const annotationObject = JSON.parse(ipfsDataJSON);
@@ -34,8 +54,33 @@ const ActionsPageFeedItemWithIPFS = ({
return undefined;
}, [annotation, ipfsDataJSON]);

const annotationMessage = getAnnotationMessage();
const location = useLocation();
// trouble connecting to IPFS
if (!ipfsDataJSON) {
return (
<CalloutCard
label={MSG.warningTitle}
labelValues={{
span: (chunks) => (
<span className={styles.noIPFSLabel}>{chunks}</span>
),
}}
description={MSG.warningText}
descriptionValues={{
link: (
<Link
to={location.pathname}
text={MSG.reloadLink}
className={styles.link}
onClick={() => window.location.reload(false)}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need false, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arrenv correct. Its superfluous - I will remove.

/>
),
}}
/>
);
}

const annotationMessage = getAnnotationMessage();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a spinner loader while trying to fetch / parse the IPFS hash (and subsequent object)

/*
* This means that the annotation object is in a format we don't recognize
*/