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
feat: Auto-focus and select file name for create new spec modal #22284
Changes from 5 commits
4fd9ed4
2c836a2
b3eab6d
7b99f29
da055b9
cfb75d4
710dfbc
5de7ed0
50851bd
f7fd80b
ab1e40d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
<div class="p-24px w-720px"> | ||
<Input | ||
v-model="specFile" | ||
:input-ref="inputRefFn" | ||
:placeholder="t('createSpec.e2e.importEmptySpec.inputPlaceholder')" | ||
:aria-label="t('createSpec.e2e.importEmptySpec.inputPlaceholder')" | ||
:has-error="hasError" | ||
|
@@ -123,7 +124,7 @@ | |
</template> | ||
|
||
<script setup lang="ts"> | ||
import { ref, watch, computed } from 'vue' | ||
import { ref, watch, computed, onMounted } from 'vue' | ||
import { useI18n } from '@packages/frontend-shared/src/locales/i18n' | ||
import Input from '@packages/frontend-shared/src/components/Input.vue' | ||
import Button from '@packages/frontend-shared/src/components/Button.vue' | ||
|
@@ -180,6 +181,10 @@ const emits = defineEmits<{ | |
|
||
const { title } = useVModels(props, emits) | ||
|
||
const inputRef = ref<HTMLInputElement>() | ||
|
||
const inputRefFn = () => inputRef | ||
|
||
const specFile = ref(props.specFileName) | ||
|
||
const matches = useMutation(EmptyGenerator_MatchSpecFileDocument) | ||
|
@@ -190,6 +195,24 @@ const hasError = computed(() => !isValidSpecFile.value && !!specFile.value) | |
|
||
const result = ref<GeneratorSuccessFileFragment | null>(null) | ||
|
||
onMounted(() => { | ||
// 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. | ||
// For example 'path/to/spec/spec-name.cy.tsx' ---> 'spec-name' | ||
const fileNameRegex = /[ \w-]+(?=\.)/ | ||
ZachJW34 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const inputValue = inputRef.value?.value | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is We have multiple onMounted(() => {
if (!inputRef?.value) {
return
}
// the stuff
}) That way, we don't need to keep checking. If |
||
const match = inputValue?.match(fileNameRegex) | ||
|
||
if (inputValue && match) { | ||
const startSelectionIndex = match.index || 0 | ||
const endSelectionIndex = inputValue.indexOf(match[0]) + match[0].length | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're still doing a bit of extra work here - couldn't this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah you're right, it could be 👍🏻 |
||
|
||
inputRef.value?.setSelectionRange(startSelectionIndex, endSelectionIndex) | ||
} | ||
}) | ||
|
||
whenever(result, () => { | ||
title.value = t('createSpec.successPage.header') | ||
emits('updateTitle', t('createSpec.successPage.header')) | ||
|
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.
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:
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 beMore 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 thinksThere 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.
Good points! No right or wrong here.
For now, happy with the unhappy case you've added: 50851bd
✅