-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update Test Reports, Test Queue and Test Run pages #419
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good just a couple little comments
9c27ecf
to
c1f0018
Compare
…os when server-side functionality added
…ersion and BrowserVersion
…ith remove-test-plan-target-migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to continue reviewing this, I feel like I only got through 10% of all the capabilities we've added and I know a few more are incoming too. Here are a couple of thoughts I did have which hopefully are helpful.
onChange={handleDateAvailabilityTextChange} | ||
onKeyPress={handleDateAvailabilityTextKeyPress} | ||
maxLength={10} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to sidestep the validation by pasting in some arbitrary text. This is admin-only so it may or may not be worth the effort to account for that.
Also you can hit submit at any time, even if the admin forgot to type in a date (in which case there is an internal error). This also may or may not be worth the effort to address. I do know keeping this component simple was a goal of the design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alflennik good points! I had initially overlooked such validation at the time with the plan to return to it and never did, so thanks for pointing that out.
I've now pushed commits which should include proper feedback to these situations. Please check again when you can
@@ -204,7 +249,7 @@ const AtAndBrowserDetailsModal = ({ | |||
</ModalSubtitleStyle> | |||
</legend> | |||
{/* Tester Scenario 1 */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards removing these "scenario x" labels because I think issue 406 won't be actively maintained going forward, but the code will be. So I predict there'll be future scenarios or alternate scenarios that will make the scenario numbers lose their usefulness. That said, Isaac does have some pretty extensive documentation for the feature so I could be swayed to change my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test. Please ignore, Howard 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another test.
a7a3907
to
48f7025
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implements sweeping changes across the Test Reports related pages, Test Queue and Test Run pages. It accounts for GraphQL server-based changes and numerous design related changes as dictated by #406.
Components which have been added/changed based on such includes:
Known Issues