Skip to content

fix(issue): Consolidate issue link parsing#463

Merged
housseindjirdeh merged 1 commit into
gitpoint:masterfrom
chinesedfan:fix462
Oct 11, 2017
Merged

fix(issue): Consolidate issue link parsing#463
housseindjirdeh merged 1 commit into
gitpoint:masterfrom
chinesedfan:fix462

Conversation

@chinesedfan
Copy link
Copy Markdown
Member

@chinesedfan chinesedfan commented Oct 10, 2017

Fixes Discovered in #462.

The regular expression can't match anything if issueURL is a issue comment, like https://github.com/octokit/discussions/issues/7#issuecomment-327616339. It causes [1] throws an error.

const issueNumberRegex = /issues\/([0-9]+)$/;
const { issue, issueURL, isPR, language } = navigation.state.params;
const number = issue ? issue.number : issueURL.match(issueNumberRegex)[1];

@machour
Copy link
Copy Markdown
Member

machour commented Oct 10, 2017

Nice catch buddy, but I think you're fixing another unknown bug here.

#462 is a bug happening when reading an issue. There's no navigation problem related there, but an Animated related exception (see my comments on that issue). If I dismiss the red screen, I do see the issue screen along with some of its comments. The bug there is supposedly with a TouchOpacity element. Still haven't investigate it yet.

@machour
Copy link
Copy Markdown
Member

machour commented Oct 10, 2017

Ok, I understood what happened here :D

There is the bug reported in #462, which is still un-resolved.
And there's a new bug you spotted by clicking my comment on #462.

@machour machour changed the title fix(issue): Fix links to issue comments fix(issue): Consolidate issue link parsing Oct 10, 2017
@chinesedfan
Copy link
Copy Markdown
Member Author

@machour Right. I saw the crash you mentioned for a while, but was attracted by this bug. 😆

I have changed the PR description so that this PR doesn't close #462.

Copy link
Copy Markdown
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Thanks a million for this @chinesedfan, and good catch :O :O

@housseindjirdeh housseindjirdeh merged commit 127deee into gitpoint:master Oct 11, 2017
@chinesedfan chinesedfan deleted the fix462 branch October 13, 2017 14:27
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