feat: Auto-focus and select file name for create new spec modal #22284
Conversation
astone123
requested review from
marktnoonan,
mike-plummer,
lmiller1990 and
ZachJW34
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ 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 |
|
@mapsandapps Any thoughts on this? #22284 (review) We had talked earlier about maybe changing the highlight color. I'm guessing we'd want this to be consistent across the entire app though, so it might be worth creating a separate issue for this |
|
|
||
| if (inputValue && match) { | ||
| const startSelectionIndex = match.index || 0 | ||
| const endSelectionIndex = inputValue.indexOf(match[0]) + match[0].length |
There was a problem hiding this comment.
I think you're still doing a bit of extra work here - couldn't this inputValue.indexOf(match[0]) be replaced by startSelectionIndex?
There was a problem hiding this comment.
yeah you're right, it could be 👍🏻
I checked with Ryan, and it should be indigo-100 edit: link to designs |
| // This regex matches a group of word characters, spaces, and '-' characters before a '.' character. | ||
| // For example 'path/to/spec/spec-name.cy.tsx' ---> 'spec-name' | ||
| const fileNameRegex = /[ \w-]+(?=\.)/ | ||
| const inputValue = inputRef.value?.value |
There was a problem hiding this comment.
Is inputRef?.value.value the same as props.specFileName? If so, we should grab it there - better not to rely on a DOM element for the source of truth. I could be wrong about this, though.
We have multiple inputRef.value? calls - seems like we should just do
onMounted(() => {
if (!inputRef?.value) {
return
}
// the stuff
})That way, we don't need to keep checking. If inputRef isn't defined for any reason, it seems we don't want to do any of the other behavior here, anyway.
| // Focus text input and try to pre-select file name | ||
| inputRef.value?.focus() | ||
|
|
||
| // This regex matches a group of word characters, spaces, and '-' characters before a '.' character. |
There was a problem hiding this comment.
As a nit/discussion item but not a hard blocker, I want to get your opinion on how we should be testing this kind of code, specifically the "business logic" part, which is parsing the spec.
Eg, I wonder if a pure function in a separate module (say 'specModal.ts', or whatever) like this would be better:
const fileNameRegex = /[ \w-]+(?=\.)/
export function getHighlightRange (filename: string) {
const match = inputValue?.match(fileNameRegex)
if (!match) {
return null
}
const startSelectionIndex = match.index || 0
const endSelectionIndex = startSelectionIndex + match[0].length
return [startSelectionIndex, endSelectionIndex]
}It would be easy to test in isolation (no need for the UI framework to come into play), and against lots of different strings - right now, we only test it against one string, which imo isn't ideal for regexp - I'd like to at least see a case where the string doesn't match, and see what happens.
We should test for the unhappy path, too, not just the happy path, and this would be an easy way to express many of those. I'm not sure the ideal of re-mounting a component many times for lots of regexp test is ideal.
Finally, you'd the import this into the component, and your onMount would be
onMounted(() => {
if (!inputRef?.value) {
return
}
const match = getSelectionRange(specFile)
if (!match) {
return
}
inputRef.value.setSelectionRange(match[0], match[1])
})More a point of discussion around best practices and testing philosophy in general, thoughts? It's a bit more overhead, but I like the separation of concerns.
Again this change isn't blocking the PR - it's more to solicit discussion and help guide us as we define the best practices for component tests, and so we can educate the rest of the company, and our users.
There was a problem hiding this comment.
Yeah this is a good discussion to have. I like the idea of having at least one component test that tests the "happy path" just so that we are sure that in most cases the component is behaving as expected. Having at least one test that covers everything together is crucial imo. But I also have this same thought
I'm not sure the ideal of re-mounting a component many times for lots of regexp test is ideal.
One thing I'd question about that though: is the regex the only thing we'd be testing? I'd argue that it's not. What's important is the experience for the user for different situations (i.e. when the default file path that we provide doesn't match the regex) which I think we can get better from a CT.
So at this point my thought is to add more component test cases that provide different default strings and see how the component behaves. That's probably what I'll do to finish out this PR, but I'm not sure if I love that approach because it doesn't seem very efficient.
I also like the idea of separating the logic out into its own function like getSelectionRange for the sake of cleaning things up a bit if nothing else. As far as using CT vs a normal unit test for something like this, I think I'm torn. Would like to hear what the rest of the team thinks
There was a problem hiding this comment.
|
Planning on addressing the selection highlight color in this issue |
…ess-io#22284) * feat: Auto-focus and select file name for create new spec modal * feat: Add comment for regex * feat: Remove comma; add new line * feat: Remove unnecessary function * feat: Remove unnecessary logic * feat: Reference prop value rather than input ref value * feat: Add component test for unhappy path Co-authored-by: Zachary Williams <ZachJW34@gmail.com>
User facing changelog
Opening the create spec modal will automatically focus the input and select the file name of the new spec
Additional details
This change is an improvement to the UX for creating a new spec. Most of the time, the user will probably want to edit the file name to override the default. By auto-focusing the input and selecting the part of the file path that the user will most likely want to edit, we can speed up the spec creation process resulting in a more convenient UX. See related issue for more info.
Steps to test
How has the user experience changed?
After
PR Tasks
cypress-documentation?type definitions?