FEATURE: Featured topic for user profile & card

[markvanlan]

No description provided.

[discoursebot]

You've signed the CLA, markvanlan. Thank you! This pull request is ready for review.

[markvanlan]

@awesomerobot Can see what you think of the changes to this html and this css?

The user card right now has all its rows numbered (from first-sixth), but all the rows following the first and second are styled no differently. I needed to add another row, and this structure made it so annoying to arbitrarily increase the class names from "third" to "fourth" and so on.

This change does not alter the user card appearance, until the user "features a topic" on their profile

[awesomerobot]

Yeah moving away from the numbered rows makes sense...

I suspect that some sites use those numbered classes to hide specific sections of the user card, but it looks like the children of those divs can be used to the safe effect? Instead of hiding .third-row hiding .location-and-website should work the same way (most of the time anyway).

[markvanlan]

@awesomerobot Yeah, all those rows have classes like .location-and-website, and I would be shocked if themes would access those elements by row rather than that class.

[markvanlan]

@eviltrout

If you created the topic you are viewing, this Feature On Profile button will now appear.

Now the Featured Topic link appears in the user card.

As well as the user's profile (this is the view logged in as someone else. This view looks different if you are viewing yourself, and does not show the link.)

And finally, users can see their featured topic under preferences -> profile

Screenshot from 2019-12-04 14-51-23@2x

[discoursereviewbot]

SamSaffron posted:

Is there a bit more context where this feature is discussed?

[discoursereviewbot]

Mark VanLandingham posted:

https://meta.discourse.org/t/add-a-curated-personal-learning-page-for-users/132897/17?u=eviltrout

basically people would like to feature a particular forum topic on their user card

[discoursereviewbot]

SamSaffron posted:

Got it, maybe lets discuss the UX a bit more on meta, I feel like the full title + the text "featured topic:" is mega noisy in then featuring scenario.

In some ways when you feature stuff like this you probably want to select a font awesome icon (with a default) and then a few short words...

🔧 buy/sell

Then we can display this link next to website on that line.

[markvanlan]

When this error has failed in the past, the error is because the test itself broke; it did not say the error message it was supposed to. It was the cannot modify frozen string error.

[eviltrout]

I believe this is unrelated and can be safely ignored.

[markvanlan]

I added to the user serializer, so this test failed.

This is the error message I got:

Failures:
  1) WebHookUserSerializer should only include the required keys
     Failure/Error: message << "#{difference} key(s) have been added to this serializer."
     
     FrozenError:
       can't modify frozen String
     # ./spec/serializers/web_hook_user_serializer_spec.rb:34:in `block (3 levels) in <main>'
     # ./spec/serializers/web_hook_user_serializer_spec.rb:28:in `block (2 levels) in <main>'

With this change, the new error message would be (if I didn't fix the error)

Failures:

  1) WebHookUserSerializer should only include the required keys
     Failure/Error:
       expect(difference).to eq(0), lambda {
         message = (difference < 0 ?
                   "#{difference * -1} key(s) have been removed from this serializer." :
                   "#{difference} key(s) have been added to this serializer.") +
                   "\nPlease verify if those key(s) are required as part of the web hook's payload."
       }

       1 key(s) have been added to this serializer.
       Please verify if those key(s) are required as part of the web hook's payload.
     # ./spec/serializers/web_hook_user_serializer_spec.rb:28:in `block (2 levels) in <top (required)>'

[eviltrout]

Oh right if you add to to the user serializer this is legit. I personally hate this test that fails every time we do that. You have to account for the new column in this test.

[eviltrout]

A nice start! Let's keep iterating on it!

[jjaffeux]

Could be simplify by:

this.get("currentUser.featured_topic.id") !== this.model.id

[markvanlan]

These statements are not equivalent, and does not work for what I need.

I do not want to entire this branch of the if, when the featured_topic is null. this.get("currentUser.featured_topic.id") !== this.model.id evaluates to true when the featured_topic is null.

[eviltrout]

Looks good to me now! Thanks.