Skip to content

Add event detail - eventType to 'remote-input-success' customEvent#34

Merged
jonrohan merged 12 commits into
mainfrom
kendallg/add-event-detail-eventType
Apr 8, 2024
Merged

Add event detail - eventType to 'remote-input-success' customEvent#34
jonrohan merged 12 commits into
mainfrom
kendallg/add-event-detail-eventType

Conversation

@kendallgassner

@kendallgassner kendallgassner commented Mar 28, 2024

Copy link
Copy Markdown
Contributor

What

We need to announce the number of results when a remote-input has finished fetching results. This announcement should only happen on input change and therefore I need to know what triggered the 'remote-input-success' customEvent.

Pairing with @TylerJDev to fix broken karma setup, ensure successful tests runs, and create a workflow to run the tests on PRs. He graciously offered to work on this pr as I became swamped on Thursday!

@kendallgassner kendallgassner requested a review from a team as a code owner March 28, 2024 16:17
@kendallgassner

Copy link
Copy Markdown
Contributor Author

I was having trouble running testing locally due to dependencies being so old. I will keep trying locally but I was hoping the tests ran in the PR 😭

Comment thread src/index.ts Outdated
const fetch = fetchResults.bind(null, this, true)
const state = {currentQuery: null, oninput: debounce(fetch), fetch, controller: null}
const fetch = (e: Event) => fetchResults.bind(null, this, true, e)
const state = {currentQuery: null, oninput: (e: Event) => debounce(fetch(e)), fetch, controller: null}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this would be better:

Suggested change
const state = {currentQuery: null, oninput: (e: Event) => debounce(fetch(e)), fetch, controller: null}
const state = {currentQuery: null, oninput: debounce((e: Event) => fetch(e)), fetch, controller: null}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point!

Comment thread test/test.js Outdated
@kendallgassner

kendallgassner commented Mar 29, 2024

Copy link
Copy Markdown
Contributor Author

The PR to update linting and add it to the workflow that runs on pull requests:

Comment thread test/test.js Outdated
Comment thread test/test.js Outdated
@kendallgassner kendallgassner requested a review from jonrohan April 8, 2024 15:34
@kendallgassner

Copy link
Copy Markdown
Contributor Author

I am not authorized to merge this pull-request. Not sure if only Primer can... but this is good to go!

@jonrohan jonrohan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems good, only curious about the eslint dependency

Comment thread .github/workflows/pr.yml
- name: Run build
run: npm run build
- name: Run test
run: npm run test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can't believe we weren't doing this already 🤦🏻

Comment thread package.json
"chai": "^4.1.2",
"chromium": "^3.0.3",
"eslint": "^6.8.0",
"eslint-plugin-github": "^3.4.1",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did we loose eslint?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah never mind, found the update pr 👍🏻

@kendallgassner kendallgassner Apr 8, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good q @jonrohan! These will be added back in #35

@kendallgassner kendallgassner Apr 8, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also, I am not authorized to merge so feel free to merge whenever.

@jonrohan jonrohan merged commit af06374 into main Apr 8, 2024
@jonrohan jonrohan deleted the kendallg/add-event-detail-eventType branch April 8, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants