community-platform icon indicating copy to clipboard operation
community-platform copied to clipboard

fix: used theme specific icons for fixing-fashion

Open asheerrizvi opened this issue 3 years ago • 12 comments

PR Checklist

PR Type

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] Developer experience (improves developer workflows for contributing to the project)

Description

Theme specific icons for fixing-fashion

Git Issues

Closes #1948

Screenshots/Videos

image

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of a monthly dev call (first Monday of the month, open to all!).

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

asheerrizvi avatar Oct 05 '22 10:10 asheerrizvi

@davehakkens We don't have all the assets for every theme present, for example precious plastic assets are being used in project kamp as well. If we can have those assets then I can add theme specific checks for each of these assets.

asheerrizvi avatar Oct 05 '22 10:10 asheerrizvi

Hey thanks I could provide the assets for (kamp) one so we can add the theming for all? Seems worth it to do it all the way.

Also if i see it correctly running your PR locally The icon you use for Precious Plastic is this one afbeelding But it should be this one (without text around it, optimised for small screens) afbeelding

davehakkens avatar Oct 05 '22 12:10 davehakkens

Visit the preview URL for this PR (updated for commit e319b89):

https://onearmy-next--pr1978-fix-1948-mljeww6u.web.app

(expires Mon, 07 Nov 2022 08:56:37 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

github-actions[bot] avatar Oct 05 '22 12:10 github-actions[bot]

@davehakkens sure let me have it and I'll add it for all 😃

asheerrizvi avatar Oct 05 '22 14:10 asheerrizvi

OK cool. So here are the .svg in a zip Icons PK.zip For large and small display size.

But I noticed the names in the themes repository are a bit mixed. So i've added an image that gives the overview of how it should be.

Explenation icons

davehakkens avatar Oct 05 '22 17:10 davehakkens

@davehakkens can you check now? Let me know if you need any changes.

asheerrizvi avatar Oct 08 '22 08:10 asheerrizvi

Nice. Just curious how do you test this one with all the different themes, locally? Annd looks like a test is failing @asheerrizvi?

davehakkens avatar Oct 09 '22 18:10 davehakkens

Hey @davehakkens, used this theme select element to switch themes on local. I think this is visible when you are running it in dev mode:

image

I'll check out the test tomorrow I suppose.

asheerrizvi avatar Oct 10 '22 16:10 asheerrizvi

any luck on the test @asheerrizvi?

davehakkens avatar Oct 15 '22 17:10 davehakkens

Hey @davehakkens, I'll push it tomorrow. Was a bit busy these last few days.

asheerrizvi avatar Oct 15 '22 18:10 asheerrizvi

@davehakkens I have fixed the tests. However, I think we will need verified member assets for each theme as well. Currently for verified member the only asset we have is based on Precious Plastic member icon. Should we do it now only or come back to it later?

image

asheerrizvi avatar Oct 16 '22 07:10 asheerrizvi

Visit the preview URL for this PR (updated for commit 56333d4):

https://onearmy-next--pr1978-fix-1948-mljeww6u.web.app

(expires Tue, 22 Nov 2022 11:52:30 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59

github-actions[bot] avatar Oct 16 '22 07:10 github-actions[bot]

We won't have verified members @asheerrizvi, it's only for spaces. We might have a verified FF and PK Space. But not anytime soon. So we could also leave it like this for now and add those later.

davehakkens avatar Oct 19 '22 12:10 davehakkens

@davehakkens cool, can you have a look and see if this can be merged?

asheerrizvi avatar Oct 21 '22 07:10 asheerrizvi

This one is hard to test locally because it contains different themes and some element can only be loaded if the data is there. For instance a Fixing Fashion Space and Project Kamp member dont exist in our dev database or emulator.

For now i'll merge it so it gets deloyed to all the devsites and we can test it! Which means it might become an iterative proces, hope thats ok :)

davehakkens avatar Oct 21 '22 11:10 davehakkens

@davehakkens yep, no worries. It was difficult for me to test on local as well. Let me know if anything seems out of ordinary.

asheerrizvi avatar Oct 21 '22 11:10 asheerrizvi



Test summary

47 0 0 0Flakiness 0


Run details

Project onearmy-community-platform
Status Passed
Commit 56333d440a
Started Oct 21, 2022 6:17 PM
Ended Oct 21, 2022 6:20 PM
Duration 02:45 💡
OS Linux Ubuntu - 20.04
Browser Chrome 106

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

cypress[bot] avatar Oct 21 '22 18:10 cypress[bot]

Hmm seems to fail when deploying to all 3 instances. https://app.circleci.com/pipelines/github/ONEARMY/community-platform/3211/workflows/78509865-77a1-488b-9538-00e2fb3cb62c

error here afbeelding

davehakkens avatar Oct 21 '22 23:10 davehakkens

@davehakkens not sure why it is erroring out for these two files. Building and serving it locally seems to work and also the deploy previews were working earlier in the PR :/

Weirdly my IDE is not flagging for these modules as missing, it's able to import them:

image image

asheerrizvi avatar Oct 22 '22 06:10 asheerrizvi

I can also try importing these using a relative path to see if that works. But I am not sure why is this causing an error for these two files only while PlatformTheme has been imported in a similar manner in other files like Sprites.tsx, Workspace.tsx etc.

asheerrizvi avatar Oct 22 '22 08:10 asheerrizvi

Got some info for you on this @asheerrizvi These are the relevant error messages:

ERROR in /home/circleci/project/src/modules/profile/index.ts
[tsl] ERROR in /home/circleci/project/src/modules/profile/index.ts(3,36)
      TS2307: Cannot find module 'src/themes/types' or its corresponding type declarations.
ERROR in /home/circleci/project/src/modules/profile/SupportedProfileTypesFactory.ts
[tsl] ERROR in /home/circleci/project/src/modules/profile/SupportedProfileTypesFactory.ts(19,36)
      TS2307: Cannot find module 'src/themes/types' or its corresponding type declarations.

The issue here is taking place during the build process for the serverless functions. It looks like the changes you have introduced here have changed the dependency tree which is no resulting in broken tests.

You should be able to run yarn workspace functions build to recreate the behaviour which is breaking in CI. This will help to determine the source of this breakage

davehakkens avatar Oct 22 '22 15:10 davehakkens

@davehakkens awesome, was able to fix it with this. Here's the PR: #1993

Some things to note:

  • Moved all profile related imports to the ..profile/types.ts file
  • Updated all files which were importing these types from ..profile/index.ts to import them from ..profile/types.ts file.

asheerrizvi avatar Oct 22 '22 16:10 asheerrizvi

cool thanks for the changes. Let's see 🤞

davehakkens avatar Oct 22 '22 17:10 davehakkens

Just tested on devsites. In case you ever need them PP = https://dev.onearmy.world PK = https://dev.community.projectkamp.com FF = https://dev.community.fixing.fashion

Works all good, thanks a lot for the cleanup @asheerrizvi

davehakkens avatar Oct 22 '22 18:10 davehakkens

:tada: This PR is included in version 1.29.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

onearmy-bot avatar Oct 23 '22 11:10 onearmy-bot

Found a little bug on this one @asheerrizvi in profile/settings The member icon is wrong in this overview. It shows the small, not large one. afbeelding

davehakkens avatar Nov 01 '22 18:11 davehakkens

@davehakkens got you. Will push a fix tomorrow.

asheerrizvi avatar Nov 01 '22 18:11 asheerrizvi

little reminder on this @asheerrizvi Bug is still there

davehakkens avatar Nov 22 '22 22:11 davehakkens

@davehakkens ahh slipped my mind. Here you go: https://github.com/ONEARMY/community-platform/pull/2003

asheerrizvi avatar Nov 23 '22 05:11 asheerrizvi

:tada: This issue has been resolved in version 1.31.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

onearmy-bot avatar Nov 26 '22 23:11 onearmy-bot