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

Assessments: Style free responses assessments question #23515

Merged
merged 15 commits into from Jul 6, 2018

Conversation

nkiruka
Copy link
Contributor

@nkiruka nkiruka commented Jul 4, 2018

The free response assessments table has question text above the table. When the question exceeds the number of characters to fit within two lines, the question is truncated and a link to view the full question is visible (see full question).

If the question does not exceed the question character limit, the link to view the full question is hidden.

Screen shot

  • Before:

screen shot 2018-07-03 at 10 37 38 pm

Screen shot

  • Post implementation

screen shot 2018-07-03 at 10 46 47 pm

Copy link
Contributor

@caleybrock caleybrock left a comment

Choose a reason for hiding this comment

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

I have a few style suggestions (that should apply to both components).

@@ -24,7 +34,11 @@ class FreeResponseBySurveyQuestionContainer extends Component {
<h2>{i18n.studentFreeResponseAnswers()}</h2>
{freeResponsesByQuestion.map((question, index) => (
<div key={index}>
<h3>{`${question.questionNumber}. ${question.questionText}`}</h3>
<div style={styles.text}>
<span>{`${question.questionNumber}. ${question.questionText}`.slice(0, QUESTION_CHARACTER_LIMIT)}</span>
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's cleaner to write this like this because we care mostly about slicing the question text:

{`${question.questionNumber}. ${question.questionText.slice(0, QUESTION_CHARACTER_LIMIT)}`}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

<h3>{`${question.questionNumber}. ${question.questionText}`}</h3>
<div style={styles.text}>
<span>{`${question.questionNumber}. ${question.questionText}`.slice(0, QUESTION_CHARACTER_LIMIT)}</span>
{((question.questionText.length >= QUESTION_CHARACTER_LIMIT))
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's a bit cleaner to write this line like this:

{ question.questionText.length >= QUESTION_CHARACTER_LIMIT &&
  <a href="#"><span>{i18n.seeFullQuestion()}</span></a>
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

ditto @caleybrock's comments, otherwise this looks great!

Copy link
Contributor

@caleybrock caleybrock left a comment

Choose a reason for hiding this comment

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

Since this link doesn't actually do anything, and we have people using this feature, we can't merge until we add that functionality. Happy to pair on this with you.

@caleybrock
Copy link
Contributor

Hmmm something went wrong with this merge conflict

@nkiruka nkiruka force-pushed the style-free-responses-assessments-question branch from 24f46db to df655fc Compare July 6, 2018 20:36
@nkiruka nkiruka merged commit bb4e4e4 into staging Jul 6, 2018
@nkiruka nkiruka deleted the style-free-responses-assessments-question branch July 6, 2018 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants