-
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
fix: Include additional "Raise an Issue" links #1281
base: development
Are you sure you want to change the base?
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.
Great work! I tested the TestRun and IssuesTable links. Both work well. The code is straightforward and easy to read. I left two small comments, one is a layout nit and the other is a musing on user flow. Neither are blocking. Thanks for this work! I am going to approve now and leave to you to merge.
flex-direction: column; | ||
align-items: flex-start; | ||
text-align: left; | ||
margin-bottom: 0.5rem; |
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 very tiny nit but I think this benefits from more margin on the bottom. It feels less cramped and makes the radio sections and the buttons more distinct. This might be a personal preference.
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.
No issue with this. I increased by a somewhat small amount because I felt it shouldn't go too far away from the related 'Raise an Issue' button. But that may also be my preference slipping in as well ha. Any thoughts on an exact value you may prefer here?
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.
That works! Thanks for the compromise between our preferences haha.
@@ -967,9 +985,62 @@ const TestRun = () => { | |||
<div role="complementary"> | |||
<h2 id="test-options-heading">Test Options</h2> | |||
<ul className="options-wrapper" aria-labelledby="test-options-heading"> | |||
<li> | |||
<li className="raise-issue-container"> |
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 know this matches a definition in the issue but I can't help but feel like this would make for a challenging user experience, especially on mobile. I almost feel like we want to move this radio into a modal or have a separate link for each command in a future design. For now this works well.
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.
move this radio into a modal
I started with a modal funny enough, but it felt like an additional layer that was just too uncommon here with how raising issues have been done in the past (single click to GitHub). It started feeling like an issue builder and I just wanted to reduce some clicks where necessary. This way preserved the default single click and only needed additional clicks if absolutely necessary. But it's something we can probably get some feedback on before even pushing to prod though.
or have a separate link for each command
(also really like that idea of there just being separate links for each command and I now prefer this in the spirit of single clicks, ha)
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.
That is a good point. I do like the thought of keeping consistency in a single-click approach. The separate link might have some navigational advantages in that it would reduce the distance between the part of the interface with information about a command and the inputs to create the issue. No strong feelings there though and happy to let this slowly improve as it gets more user testing.
…-container .options.command`
Address #1241, particularly the comments in #1241 (comment).