Issue 221

[Colinh84]

#221

[birdofpreyru]

@Colinh84 OK, it is a solid move in the right direction, but I want some changes:

1. Don't put tags staff into /src/shared/components/buttons make a separate /src/shared/components/tags. Please call default tag component just as Tag not TagButton; also don't call its stylesheet tags.scss, let it be just default.scss.
2. What you did technically looks right for the default tag (I am reviewing just looking into the code, have not launched it), but the default tag should be that gray one, as in the challenge listings.
3. For blue / green / orange tags you make separate stylesheets, similar to primaryDesign.scss, etc. stylesheets for buttons (pay attention, you don't have to copy / paste entire stylesheets, you can just import your tags default.scss into primaryDesign.scss and override only those few style rules that are different - this is completely similar to buttons.
4. To correctly style track dependent tags, you should just do PrimaryTag component, similar to PrimaryButton, and use context styling:
   if you have declared

export const PrimaryTag =
  themr('PrimaryTag, primaryDesignTag)(GenericButton);

and you use it inside challenge details header as <PrimaryTag ... />
then to use proper style for that track you just go here: https://github.com/topcoder-platform/community-app/blob/develop/src/shared/components/challenge-detail/themeFactory.js
and ensure that the proper theme is configured for each track for primary tags (again, it will be completely similar to the buttons related code, which is commented out there, because we finally did not need to make button colors track-dependent).
5. Finally, it will be great if you add to the repo a simple demo / test page for the tags you have implemented, similar to that one we have for buttons: http://community-app.topcoder.com/examples/buttons/

I'll bump the payment for this ticket by $50 for your further efforts. Though, these changes should not be that hard.

[Colinh84]

@birdofpreyru I have implemented your requested changes, including an example page for the new Tag component and updated the jest unit tests. I rebased onto develop branch since there was a conflict with a change in the last 24 hours. With the rebase, I noticed that loading the main challenge listings page no longer works and there is an error Uncaught (in promise) TypeError: Cannot read property 'filter' of null in the console; I retested on the develop branch without my changes and got the same result, so it seems there is a bad commit on develop branch dealing with filters in the last day or so..

[twicoder]

PR #421 fixed this On 08/31/2017 23:01, Colin Hunt wrote: @birdofpreyru I have implemented your requested changes, including an example page for the new Tag component and updated the jest unit tests. I rebased onto develop branch since there was a conflict with a change in the last 24 hours. With the rebase, I noticed that loading the main challenge listings page no longer works and there is an error Uncaught (in promise) TypeError: Cannot read property 'filter' of null in the console; I retested on the develop branch without my changes and got the same result, so it seems there is a bad commit on develop branch dealing with filters in the last day or so.. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[birdofpreyru]

@twicoder @Colinh84 Yeah, somehow I have not noticed this small bug during the merge.
Though, it is better to wrap all that code block into a single if (query) { ... }, so I'll close #421 and push that correction myself right now.

[birdofpreyru]

@Colinh84 OK, accepted, overally good job. Though, quite many linter errors failing npm test (I've cleaned them up myself, while testing your PR). You should definitely setup ESLint and Stylelint plugins for your IDE.

Also right after merging I've noted some deviations between design specs and your styling, but I'll also fix myself, not a big deal.