[#2098] Add show more button for error messages

[jonasongg]

Fixes #2098

Proposed commit message

Add show more button for error messages

Currently, if there are a lot of error messages, it can block the rest
of the page to the point where users may thing the report failed to
generate at all.

Let's hide extra messages automatically add a show more button if there
are more to display the messages in full.

Other information

Currently, I opted for an easier JS implementation of the feature that displays the button if the number of error messages are above 4. About this, I wanted to ask what the best place would be to put a constant that defines this number (maximum number of error messages displayed without showing more).

Regarding the design of the show more button, I wanted to also get some thoughts on what would work best for RepoSense:

(default a tag, bad contrast)

(default MUI text button)

Lastly, with JS, the transition when clicking show more is visually a bit jarring as the new elements appear programmatically. If needed, I can investigate a way to better animate showing more through CSS, which would also eliminate the need to have a constant to define the number of error messages displayed by default (preliminarily, this would involve adding a transition to max-height).

[sopa301]

LGTM! I think I like the blue option more because it's distinct from the surrounding colours. Also, the rest of RepoSense also uses blue for toggles.

[asdfghjkxd]

Good work on the PR!

However, it seems like CI is failing due to some linting mistakes!

[jonasongg]

@sopa301 @asdfghjkxd Thanks for the insight! Fixed the linting issue also

[ckcherry23]

@jonasongg Thanks for tacking this issue!

About this, I wanted to ask what the best place would be to put a constant that defines this number (maximum number of error messages displayed without showing more).

For now, I think we can define it in the view file and later move it to a dedicated Vue component.

Regarding the design of the show more button, I wanted to also get some thoughts on what would work best for RepoSense.

Currently, the 'Show More' button takes up a lot of vertical space at the bottom of the error box. Moreover, we do not expect most users to click on 'Show More' so it can be more subtle. I think what would work better to fit in with the compact style of RepoSense is using a right-aligned button styled like a red link.

Lastly, with JS, the transition when clicking show more is visually a bit jarring as the new elements appear programmatically. If needed, I can investigate a way to better animate showing more through CSS, which would also eliminate the need to have a constant to define the number of error messages displayed by default (preliminarily, this would involve adding a transition to max-height).

I think the transition isn't too bad but if you're interested in implementing smoother transitions, please go ahead!

[damithc]

One other comment about the UI: I feel the fact that some errors have been omitted should be more apparent. The current Show more appears like a regular UI control that people will automatically ignore.

For example, the last line of error message could be

Remaining error messages omitted to save space. Show all ...

What do you all think? Other alternatives?

[ckcherry23]

For example, the last line of error message could be

Remaining error messages omitted to save space. Show all ...

Just some mockups for discussion:

[damithc]

Just some mockups for discussion:

Thanks @ckcherry23 I'm OK with either. Will leave you all to decide.

[jonasongg]

@ckcherry23 @damithc Added the changes as specified!

[jonasongg]

@ckcherry23 Wondering if I should add some tests in chartView_errorSummary_messageBox.cy.js to test this show more button; if I do, then I will need to add some repos to summary.json

[ckcherry23]

Yes, it would be great if we added a test case to test this behaviour!

However, adding repos to summary.json may cause other test cases to break so we try to avoid making any changes to it (the RepoSense/cypress branch) unless absolutely necessary.

In your case, would it be okay to show only 3 errors by default?

[jonasongg]

If I'm not wrong, the default cypress configuration will only result in one error message, so I'm not too sure on how best to test this!

[ckcherry23]

If I'm not wrong, the default cypress configuration will only result in one error message, so I'm not too sure on how best to test this!

@jonasongg Okay, I thought the Cypress configuration had 4 errors, which is why I suggested the default of 3.

In that case, let's make changes to the cypress branch. You can go ahead and add more repos for analysis to the branch locally and see if all the Cypress tests pass, and fix any breaking tests.

[jonasongg]

@ckcherry23 Sorry for the late update, just added the aforementioned tests!

[ckcherry23]

LGTM! Pleasantly surprised other tests didn't break because of this change.

[ckcherry23]

@jonasongg Please edit the title of the PR to include the issue number

[jonasongg]

@ckcherry23 Made the changes! Thanks

[jonasongg]

@vvidday @ckcherry23 I added the variable in data() as I'm not too sure what the idiomatic way to define constants in our Vue files are, let me know if I should change it!

[vvidday]

Tests seem to be failing due to the new title component + added error messages pushing the charts out of viewport - Might be worth looking into the visibility selectors in our tests.

chrome_AojBzTIZzQ.mp4

[jonasongg]

@vvidday thanks for pointing this out! a very simple fix for most of these tests would like you suggested to remove the .should('be.visible') assertion in the tests, but i'm not sure if this is the right way to fix them? the assertion doesn't seem to be doing much as the test would already fail if the element cannot be found. another way could be to scroll down so that the elements are visible

[sopa301]

We can either use the exist assertion or we can use scrollIntoView as you mentioned. I think it would be better to use scrollIntoView because it would more closely follow an actual user's interaction.

[jonasongg]

@sopa301 Thanks for the suggestions! I decided to go with the exist assertion because it's an easier change (and also .click() will automatically scroll once the exist assertion passes). I also renamed zoomView_rampChart.js to zoomView_rampChart.cy.js so that the test actually appears

[ckcherry23]

LGTM!

[vvidday]

Thanks for the changes, LGTM!

[asdfghjkxd]

LGTM!

[github-actions]

The following links are for previewing this pull request:

• Dashboard Preview: https://dashboard-2105-pr-reposense-reposense.surge.sh
• Docs Preview: https://docs-2105-pr-reposense-reposense.surge.sh