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

Material Design - M1 & M2 #1145

Merged
merged 5 commits into from
Apr 8, 2024
Merged

Conversation

stojanov-igor
Copy link
Contributor

Milestone Delivery Checklist

  • The milestone-delivery-template.md has been copied and updated.
  • This pull request is being made by the same account as the accepted application.
  • I have disclosed any and all sources of reused code in the submitted repositories and have done my due diligence to meet its license requirements.
  • In case of acceptance, invoices must be submitted and payments will be transferred to the Polkadot AssetHub and/or fiat account provided in the application.
  • The delivery is according to the Guidelines for Milestone Deliverables.

Link to the application pull request: w3f/Grants-Program#2159

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Mar 8, 2024

Hi, I'll take a look at this evaluation.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Mar 8, 2024

@stojanov-igor thank you for the milestone delivery. Please take a look at the two evaluation documents in this PR and provide proper answers and fixes. After that, let me know when I can continue this evaluation.

@stojanov-igor
Copy link
Contributor Author

Hi @dsm-w3f

Thank you for the evaluation and the extensive feedback you have provided. I have resolved the issues mentioned and you can find specifics below:

  • I kept the license to be the same as the original license (Unlicense)
  • The Dockerfile was updated. There seemed to be an issue with copying node_modules inside the container, but this is now resolved. Documentation was also added to the Readme.
  • The two icons are removed from the AccountSelector module. They were there as an example and were not meant to be actionable. The theme toggle is standard in React applications.
  • The styling for the TemplateModule has been improved.
  • yarn test has been fixed.
  • The Cypress tests are meant to serve as an example. I have improved the selectors for the UI elements by adding a new tag to the elements we are testing which shortened the selectors a bit. Thanks for the suggestion.
  • I cannot reproduce the misalignment on the copy icons you mentioned. On which browsers/resolution did you notice the discrepancy?
  • There is no additional logic added for the buttons on the PalletInteractor. This means that if previously you were able to send the transaction, this should still be the case. Can you check if you are meeting all the conditions for transfer?

Regarding your other observations, I agree with you. I found a few bugs that exist within the old template. Fixing these was beyond the scope of the work since I focused only on migration to Material Design and Typescript.

Feel free to continue the evaluation..

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Mar 20, 2024

@stojanov-igor thank you for the answers and fixes. The M1 is accepted but M2 still have some adjustments to perform. Please see details in the updated evaluation documents on this PR. Let me know when I can continue this evaluation.

@stojanov-igor
Copy link
Contributor Author

Hi @dsm-w3f,

Thanks again for the feedback.

  • I have published an article on Medium regarding the grant.
  • I have resolved the runtime error you mentioned. Feel free to verify.

Regarding your comment on Firefox. I use Chrome so the styling is optimized from Chromium-based browsers.

You can proceed with the evaluation. Looking forward to your comments.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Apr 2, 2024

@stojanov-igor thank you for the improvements and fixes. We are almost done with this evaluation. There is one small bug that still remain. Please see details in the evaluation document in this PR. Please let me know when I can check it again to finish this evaluation.

@stojanov-igor
Copy link
Contributor Author

Hi @dsm-w3f

Thanks for the feedback. I have converted the TextField from 'number' to 'text' type. This resolves the last small bug you found.

Let me know if you notice other issues.

@dsm-w3f
Copy link
Contributor

dsm-w3f commented Apr 2, 2024

@stojanov-igor thank you for the fix. The two milestones are accepted from my side. The PR with the evaluations is open. Great job!

Copy link
Member

@semuelle semuelle left a comment

Choose a reason for hiding this comment

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

Thanks, @stojanov-igor and @dsm-w3f. I noticed that the README still refers to the forked repository. Could you update the URLs?

@stojanov-igor
Copy link
Contributor Author

Thanks, @stojanov-igor and @dsm-w3f. I noticed that the README still refers to the forked repository. Could you update the URLs?

Hi @semuelle. Thank you for your comment.

Could you please let me know if you're referring to delivery item 0b. Documentation that Links to the forked repository README? If so, that is the correct updated documentation for the repository.

Let me know if you meant something else.

@semuelle
Copy link
Member

semuelle commented Apr 8, 2024

Could you please let me know if you're referring to delivery item 0b. Documentation that Links to the forked repository README? If so, that is the correct updated documentation for the repository.

This line (and some other lines in the readme) refers to the original repository. Shouldn't it refer to stojanov-igor/substrate-front-end-template?

@stojanov-igor
Copy link
Contributor Author

Nice catch @semuelle

I have updated the README on the branch to reflect the fork and not the original repository.

I have also created a pull request, to merge the work done to master branch in the forked repository.

Would you prefer to keep the work in my forked repository, or would also like to create a separate branch on the original repository? Let me know...

@semuelle
Copy link
Member

semuelle commented Apr 8, 2024

Would you prefer to keep the work in my forked repository, or would also like to create a separate branch on the original repository? Let me know...

I don't know how the maintainers feel about that, but it's worth asking. Perhaps @sacha-l has a minute to review the project and comment.

Copy link
Member

@semuelle semuelle left a comment

Choose a reason for hiding this comment

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

I have verified @dsm-w3f's review. Your milestones are hereby accepted, @stojanov-igor. Congrats.

@semuelle semuelle merged commit ddb88b6 into w3f:master Apr 8, 2024
2 of 3 checks passed
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.

3 participants