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

Improved Error Dialog #2953

Merged
merged 15 commits into from Jun 16, 2023
Merged

Improved Error Dialog #2953

merged 15 commits into from Jun 16, 2023

Conversation

mindmonk
Copy link
Contributor

No description provided.

Copy link
Member

@infeo infeo left a comment

Choose a reason for hiding this comment

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

Goes in the right direction, but the code clarity can be improved.

@overheadhunter overheadhunter changed the title Feature/error dialog Improved Error Dialog Jun 13, 2023
Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

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

Either use the signum function or simply adjust the expectedResult to reflect the actual difference in upvote counts: This is not limited to just -1, 0 and 1 😄

@mindmonk
Copy link
Contributor Author

Displayed view while loading:
loading

Displayed view if solution was found:
solutionFound

Displayed view if no solution was found:
noSolutionFound

Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

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

Looks very good already. Just a couple of minor remarks (and maybe one bigger question: Can we remove "third level" error code sorting altogether?)

Comment on lines 32 to 51

<!-- ✏ Please describe what happened as accurately as possible. -->

<!-- 📋 Please also copy and paste the detail text from the error window. -->

<!-- ℹ Text enclosed like this (chevrons, exclamation mark, two dashes) is not visible to others! -->

Copy link
Member

Choose a reason for hiding this comment

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

these diffs are unrelated, i.e. please undo them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes have been undone

@overheadhunter
Copy link
Member

UI design is totally sufficient to get this merged, but I guess we can in a separate PR improve the visuals if a solution was found. CC @tobihagemann

@overheadhunter
Copy link
Member

Ok, another suggestion for god level code legibility:

@DisplayName("compare error codes by root cause")
@ParameterizedTest(name = "{0} {1} {2}")
@CsvSource(textBlock = """
		Error 6HU1:12H1:0000, =, Error 6HU1:12H1:0000
		Error 6HU1:12H1:0007, =, Error 6HU1:12H1:0042
		Error 0000:0000:0000, =, Error 0000:0000:0000
		Error 6HU1:12H1:0000, <, Error 0000:0000:0000
		Error 6HU1:12H1:0000, <, Error 6HU1:0000:0000
		Error 0000:0000:0000, >, Error 6HU1:12H1:0000
		Error 6HU1:0000:0000, >, Error 6HU1:12H1:0000
		""")
public void testCompareSecondLevelMatch(String leftTitle, char expected, String rightTitle) {
	int expectedResult = switch (expected) {
		case '<' -> -1;
		case '>' -> +1;
		default -> 0;
	};
	// ...
}

@sschuberth
Copy link
Contributor

BTW, as a user, I found it a bit disturbing to actually see the surrounding triple-backticks as part of the dialog. How about not showing them in the dialog, but adding them to the clipboard contents when clicking on Copy?

@mindmonk mindmonk merged commit 9b18a17 into develop Jun 16, 2023
6 checks passed
@tobihagemann tobihagemann deleted the feature/error-dialog branch June 16, 2023 14:25
@sschuberth
Copy link
Contributor

BTW, as a user, I found it a bit disturbing to actually see the surrounding triple-backticks as part of the dialog. How about not showing them in the dialog, but adding them to the clipboard contents when clicking on Copy?

See #2963.

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

4 participants