feat(platform): add color toggle to follow button on research page by NoHara42 · Pull Request #2205 · ONEARMY/community-platform

NoHara42 · 2023-04-05T15:39:16Z

PR Checklist

- Commit messages are descriptive, it will be used in our Release Notes

PR Type

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

First PR to an open-source project, woop!
Adds the same color as the active notification icon to the thunderbolt inside any research item.

Git Issues

Closes #2200

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

NoHara42 · 2023-04-05T15:40:43Z

What is the testing philosophy here, do we test extensively?

cypress · 2023-04-05T15:54:45Z

4 failed tests on run #3177 ↗︎

4	75	3	0	0

Details:

feat(platform): add color toggle to follow button on research page
Project: onearmy-community-platform	Commit: `aab3a7b3cc`
Status: Failed	Duration: 04:32 💡
Started: Apr 5, 2023 3:50 PM	Ended: Apr 5, 2023 3:54 PM

map.spec.ts • 1 failed test • ci-chrome

View Output Video

Test		Artifacts
map > [By Admin] > should delete a map pin		`Output` `Screenshots`

settings.spec.ts • 1 failed test • ci-chrome

View Output Video

Test		Artifacts
[Settings] > [Focus Machine Builder] > [Edit a new profile]		`Output` `Screenshots`

notificationBanner.spec.ts • 1 failed test • ci-chrome

View Output Video

Test		Artifacts
[Notification Banner] > [By Authenticated user with filled profile] > [Notification Banner is visible for user with blank profile]		`Output` `Screenshots`

events.spec.ts • 1 failed test • ci-chrome

View Output Video

Test		Artifacts
[Events] > [Create an event] > [By Authenticated]		`Output` `Screenshots`

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

github-actions · 2023-04-05T15:59:24Z

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

https://onearmy-next--pr2205-feat-follow-button-c-qjjwgn3f.web.app

_{(expires Sat, 27 May 2023 13:38:02 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59}

thisislawatts · 2023-04-05T17:46:19Z

Thanks for raising the PR so quickly @NoHara42, we've some documentation on the testing approach here: https://onearmy.github.io/community-platform/Testing/overview, although it looks a little out of date. Recently I have been introducing tests at a lower level using jest and react testing library, primarily to speed up the feedback cycle whilst working.

You could look at extending the existing ResearchArticle test coverage to cover this scenario:
https://github.com/ONEARMY/community-platform/blob/master/src/pages/Research/Content/ResearchArticle.test.tsx

davehakkens · 2023-04-12T10:43:40Z

Are you able to finish this @NoHara42 or need help?

NoHara42 · 2023-04-12T13:38:19Z

@davehakkens hey Dave, I still intend to finish this up. Sorry the last few days were busier than I thought.
I'll need another couple days before I can have another go at completing the tests I wrote.

davehakkens · 2023-04-23T17:29:26Z

let me know if you are able to finish this @NoHara42. If not no worries we will take over.
Want to merge it in so we can use it for season 3 :)

NoHara42 · 2023-04-24T15:48:46Z

@davehakkens hey Dave, I will take another look right now and will comment again if I'm not able to complete it this session.

Currently working on a farm in the south of Spain so my time is a bit limited. 🐵

NoHara42 · 2023-04-24T21:13:26Z

Had some issues setting up the Dev environment on my system today, some WSL drama...

I would like to take another crack at it tomorrow, as I have a test written.

@davehakkens do you (or perhaps @thisislawatts) have some time in the evenings to reassure me that the tests I'm writing are valid and my Dev environment is setup correctly?

I am getting a bunch of errors when I run all the projects unit tests.

I have run yarn and have built all the project files.

AlfonsoGhislieri · 2023-04-25T13:04:00Z

Hi @NoHara42 what tests are you running? The yarn test:unit should be for unit tests which is what I assume you have been writing? If its a component test then should be with yarn test:components

I don't think these should be failing.. Let me know

NoHara42 · 2023-04-25T16:50:07Z

Hi @NoHara42 what tests are you running? The yarn test:unit should be for unit tests which is what I assume you have been writing? If its a component test then should be with yarn test:components

@AlfonsoGhislieri I am running unit tests, I realised the "errors" I had are just warnings:
Warning: An update to observerComponent inside a test was not wrapped in act(...).

But the suggestion this warning suggests isn't upheld in the codebase, I'll ignore it for now.

@davehakkens I've added 2 unit tests for both the "Follow" and "Following" states of the button.
Should be ready for review.

src/pages/Research/Content/ResearchDescription.tsx

AlfonsoGhislieri · 2023-04-25T21:15:37Z

src/pages/Research/Content/ResearchDescription.tsx

+                )
+                  ? 'orange'
+                  : 'black'
+              }


Issue: Would be a lot cleaner to not have these colors hardcoded, would be nicer to add them to the theme/use the theme instead (eg: subscribed: 'orange', not-subscribed: 'black'). You can find the theme files in packages/themes/src.

Let me know if you have any questions or need any help for this. BUT, if this is too much extra work and you don't have time I'm happy to approve this and add it to my TODO.

AlfonsoGhislieri · 2023-04-25T21:19:11Z

packages/components/src/Button/Button.tsx

-          <Icon glyph={props.icon} />
+          <Icon glyph={props.icon} color={props.iconColor} />
        </Flex>
      )}


Suggestion following issue above: here you should use sx to so that you can access the theme colors directly. Eg: <Icon glyph={props.icon} sx={{ color: props.iconColor }} />

I had to add sx to the IconWrapper Component directly, as it uses styled components, which didn't seem to work well with the theming I had defined.

AlfonsoGhislieri · 2023-04-25T21:24:39Z

Thanks for this @NoHara42, great to also have the tests in here! Just a small issue regarding the hardcoded colors. Would be good to add these to the themes instead eg: subscribed: 'orange' so this can be accessed more easily. But I can fix this up if you don't have the time so just let me know.

Also the commits are not passing the linting check (needs to adhere to https://www.conventionalcommits.org), you can use git rebase -i to change these. In the future you can also use yarn commit to help with making the commits through an interactive prompt

re ONEARMY#2205

…Article button

re ONEARMY#2205

…se theming

NoHara42 · 2023-04-26T18:03:15Z

@AlfonsoGhislieri Should be done with your suggestions, also refactored some repeated theme colors while I was at it.

Sorry about the force pushes, I'm not the subtlest of git history cleaners. 🥲

AlfonsoGhislieri · 2023-04-26T21:07:58Z

Thanks for this @NoHara42! Really appreciate the extra work refactoring the themeing! I just raised #2251 to fix the unit tests that were failing and made some minor changes (sorry for hijacking), just removed some uncessary themeing and the use of hyphens. Your commits are still there in the history so your hard work is not unaccounted for! 🙃

onearmy-bot · 2023-04-27T13:31:53Z

🎉 This issue has been resolved in version 1.49.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

NoHara42 requested a review from a team as a code owner April 5, 2023 15:39

github-actions bot added the Research 🧪 label Apr 5, 2023

davehakkens added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Apr 5, 2023

AlfonsoGhislieri mentioned this pull request Apr 11, 2023

Reintroduce "fix: map page missing flags" #2215

Closed

davehakkens added the Stage: 🤝 Awaiting author Waiting on action from the author label Apr 17, 2023

thisislawatts assigned AlfonsoGhislieri Apr 25, 2023

AlfonsoGhislieri suggested changes Apr 25, 2023

View reviewed changes

NoHara42 added a commit to NoHara42/community-platform that referenced this pull request Apr 26, 2023

refactor(theme): extract common colors from themes to an exportable var

c7adc46

re ONEARMY#2205

NoHara42 force-pushed the feat/follow-button-color-toggle branch from d54efe5 to be91da7 Compare April 26, 2023 17:45

NoHara42 added 4 commits April 26, 2023 19:49

feat(platform): add color toggle to follow button on research page

3e55db8

feat(platform): add tests for 'Follow'/'Following' states of Research…

12d1bf2

…Article button

refactor(theme): extract common colors from themes to an exportable var

b1304d9

re ONEARMY#2205

refactor(Icons in ResearchDescription): adapt thunderbolt button to u…

cfed652

…se theming

NoHara42 force-pushed the feat/follow-button-color-toggle branch from be91da7 to cfed652 Compare April 26, 2023 17:50

style(formatting): format fix

3d39ad2

NoHara42 requested a review from AlfonsoGhislieri April 26, 2023 18:03

AlfonsoGhislieri mentioned this pull request Apr 26, 2023

feat(platform): add color toggle to follow button on research page #2251

Merged

5 tasks

thisislawatts closed this in #2251 Apr 27, 2023

onearmy-bot added the released label Apr 27, 2023

Uh oh!

Conversation

NoHara42 commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Git Issues

Screenshots/Videos

Uh oh!

NoHara42 commented Apr 5, 2023

Uh oh!

cypress bot commented Apr 5, 2023

4 failed tests on run #3177 ↗︎

Uh oh!

github-actions bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thisislawatts commented Apr 5, 2023

Uh oh!

davehakkens commented Apr 12, 2023

Uh oh!

NoHara42 commented Apr 12, 2023

Uh oh!

davehakkens commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NoHara42 commented Apr 24, 2023

Uh oh!

NoHara42 commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlfonsoGhislieri commented Apr 25, 2023

Uh oh!

NoHara42 commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlfonsoGhislieri Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoHara42 Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

AlfonsoGhislieri Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

NoHara42 Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

AlfonsoGhislieri commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NoHara42 commented Apr 26, 2023

Uh oh!

AlfonsoGhislieri commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onearmy-bot commented Apr 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NoHara42 commented Apr 5, 2023 •

edited

Loading

github-actions bot commented Apr 5, 2023 •

edited

Loading

davehakkens commented Apr 23, 2023 •

edited

Loading

NoHara42 commented Apr 24, 2023 •

edited

Loading

NoHara42 commented Apr 25, 2023 •

edited

Loading

AlfonsoGhislieri Apr 25, 2023 •

edited

Loading

AlfonsoGhislieri commented Apr 25, 2023 •

edited

Loading

AlfonsoGhislieri commented Apr 26, 2023 •

edited

Loading