-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix: app layout + specs list review #18862
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
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Failures
Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
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.
Nice. I have just one comment that you can take or leave about truncating the text.
@@ -6,7 +6,7 @@ | |||
* outline with that, do border-transparent for the non-hocus state. | |||
*/ | |||
|
|||
const focusDefault = 'outline-none focus:border focus:border-indigo-300 focus:ring-2 focus:ring-indigo-100 focus:outline-transparent transition duration-150 disabled:hover:ring-0 disabled:hover:border-0' | |||
const focusDefault = 'outline-none focus:border focus:border-indigo-300 focus:ring-2 focus:ring-indigo-100 focus:outline-transparent transition duration-150 disabled:hover:ring-0 disabled:hover:border-transparent' |
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.
nice fix!
@@ -27,7 +27,10 @@ | |||
class="min-w-16px max-w-16px min-h-16px icon-dark-gray-300 group-hocus:icon-dark-indigo-500 max-h-16px" | |||
/> | |||
</span> | |||
<div :class="classes?.gitText"> | |||
<div | |||
class="overflow-hidden truncate" |
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 not so sure about truncating. If we don't want to make the rows taller & wrap the text, maybe we could allow horizontal scroll. Mainly I'm thinking if the data is useful at full width, it's still useful if somebody is choosing to make the window smaller - or triggers the truncate through browser zoom controls.
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.
How about :title="specName"
? You could just hover it with your mouse.
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 resolved the conflicts with the main branch and added :title
along the way, so if the text is truncated, there's at least some way to see the full title (by hovering). Happy to discuss the UI further - since the correct behavior isn't obvious from the mocks, I'm just going to merge this one up since I don't see any obvious problems.
Let's discuss how this should behave, once we have a consensus on the correct behavior, we can make another PR to amend it.
* 10.0-release: feat: improve vite DX (#18937) feat: Use plugins on config files (#18798) BREAKING CHANGE: trigger major bump BREAKING CHANGE: trigger major bump fix: fix cypress/package.json crasher fix(breaking): change circle.yml to release binary fix: build-prod-ui deps before build-prod packages feat: implement spec list tree (#18901) chore: adding 10.0-release to the circle.yml build script (#18926) feat(app): remove __vite__ route and default to unified runner (#18909) fix: app layout + specs list review (#18862) feat(app): show previous versions (#18838) feat: scaffold integration files in app (#18763) feat: add footer to the settings (#18867) fix: Exit when both --e2e and --component flags are passed in (#18855)
User facing changelog
Additional details