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.

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

fix(issue): Consolidate issue link parsing #463

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

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

None yet

4 participants