Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Auto-focus and select file name for create new spec modal #22284

Merged
merged 11 commits into from Jun 16, 2022
Merged

feat: Auto-focus and select file name for create new spec modal #22284

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4fd9ed4
feat: Auto-focus and select file name for create new spec modal
astone123 Jun 13, 2022
2c836a2
feat: Add comment for regex
astone123 Jun 13, 2022
b3eab6d
feat: Remove comma; add new line
astone123 Jun 13, 2022
7b99f29
Merge branch 'develop' into 21865-create-spec-autofocus
astone123 Jun 13, 2022
da055b9
feat: Remove unnecessary function
astone123 Jun 13, 2022
cfb75d4
feat: Remove unnecessary logic
astone123 Jun 13, 2022
710dfbc
Merge branch 'develop' into 21865-create-spec-autofocus
ZachJW34 Jun 13, 2022
5de7ed0
feat: Reference prop value rather than input ref value
astone123 Jun 14, 2022
50851bd
feat: Add component test for unhappy path
astone123 Jun 14, 2022
f7fd80b
Merge branch 'develop' into 21865-create-spec-autofocus
astone123 Jun 15, 2022
ab1e40d
Merge branch 'develop' into 21865-create-spec-autofocus
astone123 Jun 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/app/src/specs/CreateSpecModal.cy.tsx
View file
Open in desktop
Expand Up @@ -46,6 +46,18 @@ describe('<CreateSpecModal />', () => {
cy.percySnapshot()
})

it('focuses text input and selects file name by default', () => {
cy.focused().as('specNameInput')

// focused should yield the input element since it should be auto-focused
cy.get('@specNameInput').invoke('val').should('equal', 'cypress/e2e/ComponentName.cy.js')

// only the file name should be focused, so backspacing should erase the whole file name
cy.get('@specNameInput').type('{backspace}')

cy.get('@specNameInput').invoke('val').should('equal', 'cypress/e2e/.cy.js')
})

describe('dismissing', () => {
it('is dismissed when you click outside', () => {
cy.get(modalSelector)
Expand Down
28 changes: 27 additions & 1 deletion packages/app/src/specs/generators/EmptyGenerator.vue
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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)
Expand All @@ -190,6 +195,27 @@ const hasError = computed(() => !isValidSpecFile.value && !!specFile.value)

const result = ref<GeneratorSuccessFileFragment | null>(null)

function getFileNameIndexes (inputValue: string, match: RegExpMatchArray): number[] {
return [match.index || 0, inputValue.indexOf(match[0]) + match[0].length]
}

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.
Copy link
Contributor

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:

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.

Copy link
Contributor Author

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

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

Copy link
Contributor

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

✅

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const match = inputValue?.match(fileNameRegex)

if (inputValue && match) {
const [startSelectionIndex, endSelectionIndex] = getFileNameIndexes(inputValue, match)
astone123 marked this conversation as resolved.
Show resolved Hide resolved

inputRef.value?.setSelectionRange(startSelectionIndex, endSelectionIndex)
}
})

whenever(result, () => {
title.value = t('createSpec.successPage.header')
emits('updateTitle', t('createSpec.successPage.header'))
Expand Down