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

feat: support for quicking view change files in pull request view #245

Merged
merged 3 commits into from
Mar 25, 2021

Conversation

conwnet
Copy link
Owner

@conwnet conwnet commented Mar 23, 2021

Improve the user experience of Pull Request View, support for quicking view change files

@vercel
Copy link

vercel bot commented Mar 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vscode-github1s/github1s/7oYPuVqTkzEVvq2HRTiTf36Gaxz7
✅ Preview: https://github1s-git-feature-pulls-vscode-github1s.vercel.app

@conwnet
Copy link
Owner Author

conwnet commented Mar 23, 2021

@xcv58
Copy link
Collaborator

xcv58 commented Mar 23, 2021

For the CI failure, @conwnet you might want to run:

yarn build
yarn serve
yarn test -u 

on different terminals to update the screenshots. If it's related the snapshot match.

@@ -78,3 +78,12 @@ export const encodeFilePath = (filePath: string): string => {
.map((segment) => encodeURIComponent(segment))
.join('/');
};

// parse the querystring simply: 'a=1&b=2&c=3'
export const parseQuery = (query: string): { [key: string]: string } => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we use https://github.com/ljharb/qs or https://github.com/sindresorhus/query-string to handle the query parsing since there might be problems with escaped character and other edge cases.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thank for you recommendation! I wanted to use qs yesterday, but I found the bundle size of the qs looks like too big, so I write one simply. Bow I think query-string is better for browsers, I will use it~

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a great point.

I think we should set up a visible indicator of the performance, there are tools like https://github.com/webpack-contrib/webpack-bundle-analyzer that can do the code analysis, but I think it needs some setup to make sure it works with this project.

Another direction is the browser measurement like the lighthouse (I have installed it onVercel) and https://vercel.com/integrations/debugbear that can do the analysis but it needs to configure an account. Could you please take a look if you are interested & when you have time?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Another direction is the browser measurement like the lighthouse (I have installed it onVercel) and https://vercel.com/integrations/debugbear that can do the analysis but it needs to configure an account. Could you please take a look if you are interested & when you have time?

Thank you so much! I will configurate it next

Copy link
Owner Author

Choose a reason for hiding this comment

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

Another direction is the browser measurement like the lighthouse (I have installed it onVercel) and https://vercel.com/integrations/debugbear that can do the analysis but it needs to configure an account.

@xcv58 The lighthouse is working, I think we can not use the debugbear because it is not for free.😂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. Sorry I didn’t know that before. I think we can keep in mind about this and looking for measurements in the long term.

extensions/github1s/src/listeners/vscode.ts Show resolved Hide resolved
extensions/github1s/src/views/pull-list-view.ts Outdated Show resolved Hide resolved
@conwnet conwnet merged commit a9a4114 into master Mar 25, 2021
@conwnet conwnet deleted the feature/pulls branch March 28, 2021 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants