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 optional help text to contentanalysis #480

Conversation

igorschoester
Copy link
Member

@igorschoester igorschoester commented Apr 30, 2018

Summary

This PR can be summarized in the following changelog entry:

  • Added a help text component.

Relevant technical choices:

  • Uses PropType one of to make it possible to pass the text in an array as well as just a string. In the array you can pass a link for example. This prop type is exported to be used in a component that uses HelpText.
  • Uses a paragraph element with a default style as main parent.

Test instructions

This PR can be tested by following these steps:

  • View the HelpText in action: yarn start -> localhost:3333 -> tab Content analysis.
  • Check yarn test && grunt check.

Fixes #476


// Analysis collapsibles are only rendered when there is at least one analysis result for that category present.
return (
<ContentAnalysisContainer>
{ helpText && <HelpText text={helpText} /> }
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add spaces within the brackets.

changeLanguageLink={ "#" }
language="English"
showLanguageNotice={ true }
canChangeLanguage={ true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be false? it's testing for "someone who cannot change the language"

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice spot. That makes complete sense to me yes. Changing.

import colors from "../../../../style-guide/colors";

const YoastHelpText = styled.p`
color: ${colors.$color_grey_text};
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add spaces within brackets.


return (
<YoastHelpText>
{text}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add spaces within brackets.

@afercia
Copy link
Contributor

afercia commented Apr 30, 2018

CR 🚧

A few minor coding standards and one typo in the tests.

@danielFangstrom
Copy link
Contributor

CR 👍

@danielFangstrom
Copy link
Contributor

ACC 👍

@danielFangstrom danielFangstrom merged commit 3936f39 into feature/content-analysis-in-sidebar May 1, 2018
@danielFangstrom danielFangstrom deleted the stories/476-add-optional-help-text-to-contentanalysis branch May 1, 2018 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants