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

Code submission for FLIP fest 'Playground Feature: Improve the resource explorer 29 - Milestone 3' #175

Merged
merged 6 commits into from
Nov 8, 2021

Conversation

hichana
Copy link
Contributor

@hichana hichana commented Oct 28, 2021

Closes: N/A

Description

This PR contains code changes for my submission for the 3rd/final milestone on FLIP fest issue 29. It contains an implementation of a UI that will allow users to have a better experience with the resources explorer. I made a comprehensive guide with notes and instructions on how to run the updated UI locally HERE.


For contributor use:

  • [ x] Targeted PR against correct branch (see CONTRIBUTING.md)
  • [ n/a] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • [x ] Code follows the standards mentioned here.
  • [ n/a] Updated relevant documentation
  • [n/a ] Re-reviewed Files changed in the Github PR explorer
  • [n/a ] Added appropriate labels

@hichana hichana requested a review from 10thfloor as a code owner October 28, 2021 06:56
@vercel
Copy link

vercel bot commented Oct 28, 2021

@hichana is attempting to deploy a commit to the Flow Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@MaxStalker MaxStalker 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 :)
Just some small changes here and there.

src/api/apollo/generated/graphql.tsx Show resolved Hide resolved
src/components/AccountList.tsx Outdated Show resolved Hide resolved
src/components/Sidebar.tsx Outdated Show resolved Hide resolved
src/components/TemplatePopup.tsx Outdated Show resolved Hide resolved
src/layout/BottomBarItemInsert.tsx Outdated Show resolved Hide resolved
src/providers/Project/projectDefault.ts Show resolved Hide resolved
src/util/accounts.ts Outdated Show resolved Hide resolved
@hichana
Copy link
Contributor Author

hichana commented Nov 3, 2021

@MaxStalker I made changes based on your feedback and merged to this branch/PR. I left responses to two of your comments in this PR and will wait for your response to see if more changes are needed, which I'm happy to do if so. Thank you for reviewing my submission!

@kerrywei kerrywei requested a review from MaxStalker November 4, 2021 16:42
src/util/storage.ts Outdated Show resolved Hide resolved
@MaxStalker
Copy link
Contributor

@hichana can you also check that imports are using absolute paths? i.e.

import { SideBar } from "components/Sidebar"

and not

import { SideBar } from "../../components/Sidebar"

I've seen the later for components, hooks and util folders :)
Thanks in advance!

@hichana
Copy link
Contributor Author

hichana commented Nov 5, 2021

@MaxStalker I added the util alias to both the 'tsconfig.json' file and 'webpack.config.js', which then allows the util to be used as an absolute import. I changed my use of relative imports as well as the ones that existed from previous master branch. For the hooks, I'm having trouble getting that to work for some reason. Actually, I don't believe I made any new imports for hooks in my submission, but I'm happy to change the existing ones if you can help troubleshoot why adding the alias for hooks to the two config files doesn't make the absolute imports work. And finally, can you tell me where you found import { SideBar } from "../../components/Sidebar"? I went through everything file by file and can't find an instance where I did a relative import for a component. Maybe I just missed it tho.

For now, I pushed my changes to the util absolute imports on my submission branch for this PR.

Also, how to handle this for FLIP #18 -- shall I update relative imports there as well? I'll have a look to make sure we didn't use any relative imports for components, but should I implement the util relative imports, then once hooks is figured out that as well? I know all of this work will be merged, so perhaps just one (this project) is enough?

Update: the hooks absolute import actually does work, it's just that vscode underlines it as red saying it can't find the import even tho the playground compiles just fine and works. Still unsure why it does this. Will update this or not depending on your preference.

@MaxStalker
Copy link
Contributor

Looks great!
Thank you for updating the imports ;)

@kerrywei
Copy link

kerrywei commented Nov 7, 2021

@hichana if you can do a rebase, it might enable you yo merge in this PR as the CODEOWNER file has been updated (so that Max is also one of the code owners)

* Add Max as Codeowner

* Update CODEOWNERS

Co-authored-by: Mackenzie <[email protected]>
@hichana
Copy link
Contributor Author

hichana commented Nov 8, 2021

Hi @kerrywei -- I believe I did the rebase. Is it what was needed?

I also made a branch where the upstream commit is at the beginning, before my commits come it. Not sure if that's what is best here but here is the branch: https://github.com/hichana/flow-playground/commits/flip-29-final-submission_backup

@vercel
Copy link

vercel bot commented Nov 8, 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/onflow/flow-playground/3TCrbTCALvXZPyUDtivAb6PCvZjP
✅ Preview: https://flow-playground-git-fork-hichana-flip-29-final-su-05c642-onflow.vercel.app

@vercel vercel bot temporarily deployed to Preview November 8, 2021 17:38 Inactive
@kerrywei
Copy link

kerrywei commented Nov 8, 2021

@hichana it should be good to go now

@10thfloor 10thfloor merged commit aed8c3d into onflow:master Nov 8, 2021
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.

4 participants