Skip to content

ADT-211 + DEVELOP-1305-2: Refactor github extension and empty state#31

Merged
jemmyw merged 11 commits intomainfrom
ADT-211-refactor-inline-with-other-exts
Aug 25, 2022
Merged

ADT-211 + DEVELOP-1305-2: Refactor github extension and empty state#31
jemmyw merged 11 commits intomainfrom
ADT-211-refactor-inline-with-other-exts

Conversation

@jemmyw
Copy link
Collaborator

@jemmyw jemmyw commented Aug 10, 2022

This PR adds the new empty state to the GitHub extension and makes the following adjustments:

  • Converted to typescript
  • Installed and used offical octokit types
  • Used menu and button styling from the BitBucket extension

[IDENTIFIER, trigger].join("."),
true
);
if (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

typescript doesn't yet support discrimination using includes


// Generate events.
if (record) {
if (pr.head?.name) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is already done in getOrLinkPullRequestRecord

@jemmyw jemmyw marked this pull request as ready for review August 10, 2022 20:52
@jemmyw jemmyw changed the title ADT-211: Refactor github extension and add test suite ADT-211 + DEVELOP-1305-2: Refactor github extension and empty state Aug 10, 2022
Copy link
Contributor

@percyhanna percyhanna left a comment

Choose a reason for hiding this comment

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

The aha-develop.github-v1.6.0.gz file should not be committed.

@jemmyw
Copy link
Collaborator Author

jemmyw commented Aug 15, 2022

The aha-develop.github-v1.6.0.gz file should not be committed.

@percyhanna removed and .gitignore updated

Copy link
Contributor

@trydionel trydionel left a comment

Choose a reason for hiding this comment

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

Looks good from PM perspective!

@jemmyw jemmyw dismissed percyhanna’s stale review August 24, 2022 21:45

Asking for review from the rest of the team while percy is away

Comment on lines +182 to 184
return await RecordClass.select(["id", "referenceNum"]).find(
ahaReference.referenceNum
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use findBy() here to avoid throwing the UI alert if the record doesn't exist? (Returns the record or undefined if not found)

Suggested change
return await RecordClass.select(["id", "referenceNum"]).find(
ahaReference.referenceNum
);
return await RecordClass.select(["id", "referenceNum"]).findBy({
id: ahaReference.referenceNum
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't want to make this functional change right now

};

const pasteLink = async (url: string) => {
if (!validPrUrl(url)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as on the GitLab extension - new URL() throws if I put a junk string in, so the user doesn't get any validation feedback. try/catch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@jemmyw jemmyw merged commit e2c5910 into main Aug 25, 2022
@jemmyw jemmyw deleted the ADT-211-refactor-inline-with-other-exts branch August 25, 2022 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants