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 overview table #22407

Merged
merged 4 commits into from May 15, 2018
Merged

Assessments: style overview table #22407

merged 4 commits into from May 15, 2018

Conversation

nkiruka
Copy link
Contributor

@nkiruka nkiruka commented May 14, 2018

This PR is to style the overview assessments table. The following changes were made to the table:

-Ensure consistent column width of the answer columns

-Add spacing between the icon(green check mark) and text using Flexbox

-Truncate the question text and adjust widths.

Before Implementation:
screen shot 2018-05-14 at 3 24 34 pm

After Implementation:
screen shot 2018-05-14 at 3 06 38 pm

Pair programmed Nkiru/Caley

style: {
...tableLayoutStyles.cell,
...styles.questionCell,
maxWidth: styleConstants['content-width'] - (numAnswers * (ANSWER_COLUMN_WIDTH + PADDING * 2)),
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, this is the only place you use the PADDING constant. Why not set it to 20 initially so you don't have to do math?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

return this.getAnswerColumn(answer.multipleChoiceOption);
});

const numAnswerColumns = columnLabelNames.length + 1;
console.log(numAnswerColumns);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove debugging statement

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

{
name: 'Assessment multiple choice with 3 answers',
description: 'Ability to see assessment overview for the entire class',
story: () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

super tiny nit: instead of class, can we call this a section to be consistent with other references?

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.

@nkiruka
Copy link
Contributor Author

nkiruka commented May 15, 2018

@Erin007 @caleybrock Updated PR.

@nkiruka nkiruka merged commit b88af2d into staging May 15, 2018
@nkiruka nkiruka deleted the assessments-styles branch May 15, 2018 16:41
style: {
...tableLayoutStyles.cell,
...styles.questionCell,
maxWidth: styleConstants['content-width'] - (numAnswers * (ANSWER_COLUMN_WIDTH + PADDING)),
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 the maxWidth calculation could be pulled out into a function that accepts numAnswers as a parameter. that way you can give it a name and clear up this line a little 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

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.

None yet

6 participants