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(homeIconOnFiles): adjust home icon down to avoid icon clash #474

Merged
merged 7 commits into from
Dec 7, 2021

Conversation

KemoPaw
Copy link
Contributor

@KemoPaw KemoPaw commented Dec 6, 2021

What this PR does πŸ“–

https://satellite-im.atlassian.net/browse/AP-156

Which issue(s) this PR fixes πŸ”¨

Fixes #

Special notes for reviewers πŸ—’οΈ

Additional comments 🎀

@stavares843 stavares843 added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Dec 6, 2021
Copy link
Member

@stavares843 stavares843 left a comment

Choose a reason for hiding this comment

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

Captura de ecrã 2021-12-06, às 22 28 01

fixed βœ…

@stavares843 stavares843 added the QA tested One QA Person has tested - Needs QA Lead review still label Dec 6, 2021
Copy link
Contributor

@WanderingHogan WanderingHogan left a comment

Choose a reason for hiding this comment

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

This looks alright when it is collapsed, but when it is not the padding pushes everything down too far
Screen Shot 2021-12-06 at 4 05 19 PM
Screen Shot 2021-12-06 at 4 05 16 PM

Can you do something like watch for the left bar being collapsed, and only applying that padding if that is true?

@WanderingHogan WanderingHogan added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Dec 6, 2021
@KemoPaw
Copy link
Contributor Author

KemoPaw commented Dec 6, 2021

@WanderingHogan That sounds like a good suggestion, will look into implementing it!

@stavares843 stavares843 added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Dec 7, 2021
@josephmcg
Copy link
Contributor

josephmcg commented Dec 7, 2021

Functionality looks good! Small refactor request..

It seems like this was formatted using eslint rather than prettier, is that because of husky? You can configure prettier to format files on save so you don't have to think about it.
image

In this case, I think the template literals (${}) are redundant.
https://vuejs.org/v2/api/#v-bind

Could be rewritten like this
image

@stavares843
Copy link
Member

yes husky is using eslint instead of prettier, but we look into setup that to use both

we added the vue plugin for eslint as well, do you feel those template literals (${}) that you mentioned are something that should have been captured by the linter? @josephmcg

@stavares843
Copy link
Member

@josephmcg added that here
#479

@KemoPaw
Copy link
Contributor Author

KemoPaw commented Dec 7, 2021

@josephmcg Thank you so much for explaining how to clean up the classes in files.vue + have my code format to Prettier on save!

@josephmcg
Copy link
Contributor

josephmcg commented Dec 7, 2021

@stavares843
Not necessarily, there is definitely a use case for template literals inside a v-bind. I just felt it was redundant this time.

It could be used to dynamically set attributes
image

Thanks for checking!

@stavares843
Copy link
Member

thanks for clarifying! πŸŽ‰

Copy link
Contributor

@WanderingHogan WanderingHogan left a comment

Choose a reason for hiding this comment

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

Good job Kat

@WanderingHogan WanderingHogan added Ready for QA Ready for QA team to test, Devs approved. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Dec 7, 2021
@stavares843 stavares843 merged commit de7f2f2 into dev Dec 7, 2021
@stavares843 stavares843 deleted the homeIconOnFiles branch December 7, 2021 04:19
@github-actions github-actions bot removed Ready for QA Ready for QA team to test, Devs approved. QA tested One QA Person has tested - Needs QA Lead review still labels Dec 7, 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.

None yet

5 participants