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

#564 test result page changes #579

Merged
merged 6 commits into from
May 27, 2022
Merged

Conversation

cescoferraro
Copy link
Contributor

This PR addreses issues raised by #564

Changes

Test are at bottom of test result should be open when you first come to the test. Currently it is closed and difficult to notice.
Show a chevron on the test result bar to indicate it can be opened & closed
Show 'Timeline view' and 'Graph view' mouseover on the buttons for switching view.
Delete assertion & Edit assertion tooltips & change delete to a trashcan icon
Remove the #de22 label from span label

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@kdhamric
Copy link
Collaborator

If you can use Loom to create really quick demos, it will help me when looking at the changes to make sure everything looks right.

@cescoferraro
Copy link
Contributor Author

Copy link
Collaborator

@xoscar xoscar left a comment

Choose a reason for hiding this comment

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

Hey @cescoferraro good job! I left some non-blocking comments

@@ -41,6 +41,7 @@
},
"scripts": {
"start": "craco start",
"dev": "concurrently 'npm run start' 'npm run port-forward'",
Copy link
Collaborator

Choose a reason for hiding this comment

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

great!

@@ -30,7 +30,7 @@ export const SelectorListText = styled(Typography.Text).attrs({
`;

export const SpanCountText = styled(Typography.Text)`
font-size: 12;
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol good catch

@@ -9,18 +10,24 @@ interface IProps {
selectedType: SupportedDiagrams;
}

const color = '#FFFFFF';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's think about incorporating the style components theme so we don't have to hardcode colors moving forward (future work)

@@ -80,6 +80,9 @@ const TraceDrawerHeader: React.FC<IProps> = ({
>
Add Assertion
</S.AddAssertionButton>
<span style={{marginLeft: 16}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could or not, create styled-components everytime we try to style an HTML/antd tag, let's think about that for the future.

@cescoferraro cescoferraro merged commit 06c5ea9 into main May 27, 2022
@mathnogueira mathnogueira deleted the 564-test-result-page-changes branch June 25, 2022 18:53
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.

3 participants