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

Refactor/Icons refactoring #102

Closed
wants to merge 1 commit into from
Closed

Conversation

SepidehShahbazi
Copy link
Contributor

Completed the refactoring of documentPage icons for issue #50.

@SepidehShahbazi SepidehShahbazi linked an issue Nov 25, 2024 that may be closed by this pull request
@SepidehShahbazi SepidehShahbazi self-assigned this Nov 25, 2024
@SepidehShahbazi SepidehShahbazi added Refactor Code Improvement Frontend Frontend Related Issue labels Nov 25, 2024
Copy link
Contributor

@mahid797 mahid797 left a comment

Choose a reason for hiding this comment

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

Assuming you have tested each icon, I am approving these changes. But I will not merge the PR, so you can keep working on the same branch.

@gorkem-bwl
Copy link
Contributor

@SepidehShahbazi once you have a PR with a visual change, could you send images/videos supporting your work? This will also help me understand what has changed overall. Thanks!

@parwatcodes
Copy link
Member

@SepidehShahbazi once you have a PR with a visual change, could you send images/videos supporting your work? This will also help me understand what has changed overall. Thanks!

Also, If possible it's better to create the PR with UI changes for better readability, It would let reviewers know about the exact changes in UI.

UI Changes Overview

Before

Previous UI

After

Updated UI


Summary of Changes

  • Updated button styles.
  • Improved form layout.
  • Fixed responsive issues on mobile.

@mahid797
Copy link
Contributor

mahid797 commented Dec 8, 2024

@gorkem-bwl @parwatcodes

There are no UI changes in this PR as this is just a refactoring of the icons.

@gorkem-bwl
Copy link
Contributor

@gorkem-bwl @parwatcodes

There are no UI changes in this PR as this is just a refactoring of the icons.

Got it, thanks!

@SepidehShahbazi
Copy link
Contributor Author

@SepidehShahbazi once you have a PR with a visual change, could you send images/videos supporting your work? This will also help me understand what has changed overall. Thanks!

@gorkem-bwl as @mahid797 has mentioned, in this case we don't have any UI changes. It's about refactoring Icons and converting them to React components instead of .svg, which triggers the icons to be reusable, flexible and easier for using, but during implementing this PR for other icons if there are some icons that are not match with the Figma icons and I change them, which causes the UI to be changed, I'll add photos/videos in the PR.
Also, from now on when I create a PR, I can comment on the PR that we don't have any UI/visual changes here. And for those, which we have UI changes and I add some photos, I can mention you on those PRs about new or updated photos.
I think it's easier for you to recognise new UI changes.

@SepidehShahbazi
Copy link
Contributor Author

@SepidehShahbazi once you have a PR with a visual change, could you send images/videos supporting your work? This will also help me understand what has changed overall. Thanks!

Also, If possible it's better to create the PR with UI changes for better readability, It would let reviewers know about the exact changes in UI.

UI Changes Overview

Before

Previous UI

After

Updated UI

Summary of Changes

  • Updated button styles.
  • Improved form layout.
  • Fixed responsive issues on mobile.

@parwatcodes it's a good idea. From now on, I can apply it for PRs, which have UI changes.

@gorkem-bwl
Copy link
Contributor

@SepidehShahbazi once you have a PR with a visual change, could you send images/videos supporting your work? This will also help me understand what has changed overall. Thanks!

@gorkem-bwl as @mahid797 has mentioned, in this case we don't have any UI changes. It's about refactoring Icons and converting them to React components instead of .svg, which triggers the icons to be reusable, flexible and easier for using, but during implementing this PR for other icons if there are some icons that are not match with the Figma icons and I change them, which causes the UI to be changed, I'll add photos/videos in the PR. Also, from now on when I create a PR, I can comment on the PR that we don't have any UI/visual changes here. And for those, which we have UI changes and I add some photos, I can mention you on those PRs about new or updated photos. I think it's easier for you to recognise new UI changes.

That's my bad. I mistakenly thought "refactoring" somehow meant new icons :-D Normally that doesn't happen. I'll be more careful next time.

@SepidehShahbazi
Copy link
Contributor Author

@SepidehShahbazi once you have a PR with a visual change, could you send images/videos supporting your work? This will also help me understand what has changed overall. Thanks!

@gorkem-bwl as @mahid797 has mentioned, in this case we don't have any UI changes. It's about refactoring Icons and converting them to React components instead of .svg, which triggers the icons to be reusable, flexible and easier for using, but during implementing this PR for other icons if there are some icons that are not match with the Figma icons and I change them, which causes the UI to be changed, I'll add photos/videos in the PR. Also, from now on when I create a PR, I can comment on the PR that we don't have any UI/visual changes here. And for those, which we have UI changes and I add some photos, I can mention you on those PRs about new or updated photos. I think it's easier for you to recognise new UI changes.

That's my bad. I mistakenly thought "refactoring" somehow meant new icons :-D Normally that doesn't happen. I'll be more careful next time.

No worries at all ^_^. I appreciate you taking the time to check it thoroughly. Let me know if there's anything else you'd like adjusted or clarified.

@mahid797 mahid797 closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Frontend Related Issue Refactor Code Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asset and Icons Restructuring/Refactor
4 participants