Remove flare tag from title and hightlight it within the tags row #10528

akshayymahajan · 2020-10-02T08:47:38Z

What type of PR is this? (check all applicable)

Description

This PR does 2 things:

Remove the Flare Tag from the post title.
Moves the flare tag to the first position within the list of tags and highlights it.

Related Tickets & Documents

Closes #10523

QA Instructions, Screenshots, Recordings

Added tests?

yes
no, because they aren't needed
no, because I need help

Added to documentation?

docs.forem.com
readme
no documentation needed

ludwiczakpawel

Oh damn, I love it! Thank you so much! I have one very tiny change request - other than that it's awesome!

ludwiczakpawel · 2020-10-02T09:00:58Z

app/javascript/articles/components/TagList.jsx

+              color: flare_tag.text_color_hex,
+            }}
+          >
+            {`#${flare_tag.name}`}


Can you wrap # with  similar to how we do it with other tags?

Sure thing!

ludwiczakpawel · 2020-10-02T09:04:15Z

I've just realized, that when you scroll down quite a lot on your feed we actually load feed cards differently (I don't know why...) - so the feed card code is generated in multiple places. That jsx component is one of them and it works great. But we gonna need it in other places too.. Do you think you can look at it as well? You can also spot it in logged out version of the page where we also load feed card differently.

akshayymahajan · 2020-10-02T09:07:02Z

Sure.. I'll look into it..

ludwiczakpawel · 2020-10-02T09:07:45Z

@akshayymahajan awesome! I'm super excited about this one :)

Btw. here are some references you may find useful:

akshayymahajan · 2020-10-02T12:13:40Z

Hey @ludwiczakpawel, I have updated both files and tested in the logged out page and the search results. Can you check again?

ludwiczakpawel

Awesome! I left few last change request - nothing really major :)

ludwiczakpawel · 2020-10-02T12:36:21Z

app/assets/javascripts/utilities/buildArticleHTML.js

+      flareTag =
+        "<a href='/t/" +
+        article.flare_tag.name +
+        "' class='crayons-tag'>" +
+        "<span class='crayons-story__flare-tag' style='background:" +
+        article.flare_tag.bg_color_hex +
+        ';color:' +
+        article.flare_tag.text_color_hex +
+        "'><span className='crayons-tag__prefix'>#</span>" +
+        article.flare_tag.name +
+        '</span>' +
+        '</a>';


We should drop this extra container crayons-story__flare-tag - it's not needed anymore.

Suggested change

flareTag =

"<a href='/t/" +

article.flare_tag.name +

"' class='crayons-tag'>" +

"#" +

article.flare_tag.name +

'' +

'</a>';

flareTag = `<a href="/t/${article.flare_tag.name}" class="crayons-tag" style="background: ${article.flare_tag.bg_color_hex}; color: ${article.flare_tag.text_color_hex};">

#

${article.flare_tag.name}

</a>`;

The flare tag is basically now a classic crayons-tag with custom background and color.

ludwiczakpawel · 2020-10-02T12:37:29Z

app/assets/stylesheets/components/stories.scss

@@ -135,7 +135,7 @@

  &__flare-tag {


We should now be able to remove this entire class, right?

ludwiczakpawel · 2020-10-02T12:38:34Z

app/javascript/articles/components/TagList.jsx

+      {flare_tag && (
+        <a className="crayons-tag" href={`/t/${flare_tag.name}`}>
+          <span
+            className="crayons-story__flare-tag"


Similarly to what I suggested in previous comment, we should drop .crayons-stroy__flare-tag

ludwiczakpawel · 2020-10-02T12:39:33Z

app/views/articles/_single_story.html.erb

-        <% story.cached_tag_list_array.each do |tag| %>
+        <% flare_tag = FlareTag.new(story, @tag).tag %>
+        <% if flare_tag %>
+          <a href="/t/<%= flare_tag.name %>" class="crayons-tag"><span class="crayons-story__flare-tag" style="background:<%= flare_tag.bg_color_hex %>;color:<%= flare_tag.text_color_hex %>"><span className="crayons-tag__prefix">#</span><%= flare_tag.name %></span></a>


And same here.

akshayymahajan · 2020-10-02T13:11:58Z

@ludwiczakpawel updated!

ludwiczakpawel

One tiny change and it's ready!

app/assets/stylesheets/components/stories.scss

lisasy

Nice! Thank you :) This looks great.

rhymes · 2020-10-04T08:29:17Z

@akshayymahajan it looks like some of the tests are failing, please take a look at the build here: https://github.com/forem/forem/pull/10528/checks?check_run_id=1201884642

akshayymahajan · 2020-10-04T09:11:38Z

Hey @rhymes, I have updated the code to fix the failing test.

ludwiczakpawel

Hey, I've taken it for another ride and there's still something a little bit off, when I scroll down the feed. Here's a video showing the issue. Looks like on the feed cards that are being loaded with infinity scroll we don't apply appropriate colors/styling to the flare tag.

akshayymahajan · 2020-10-05T09:28:16Z

Hey @ludwiczakpawel, looks like the data being returned from the API has some null values. Not sure if this is related to the changes I made. Would really appreciate some help.

ludwiczakpawel · 2020-10-05T11:08:34Z

Interesting. I can see this working properly on prod dev.to...

@rhymes I SUMMON YOU.

rhymes · 2020-10-08T07:19:20Z

Hi @akshayymahajan, that indicates that the tag has no defined colors:

forem/app/models/tag.rb

Lines 26 to 27 in a943f04

    
           validates :text_color_hex, format: HEX_COLOR_REGEXP, allow_nil: true 
        
           validates :bg_color_hex, format: HEX_COLOR_REGEXP, allow_nil: true

both hex colors can be nil

akshayymahajan · 2020-10-08T07:24:35Z

Thanks, @rhymes. @ludwiczakpawel in that case, I think it would be best to add back the span.crayons-stroy__flare-tag wrapper in all the places. What do you say?

ludwiczakpawel · 2020-10-08T09:11:36Z

@rhymes are you sure? can you please checkout the screen recording here: https://d.pr/v/Pzx0DE - it's the same tag. one time it has colors applied and the other time it does not... which means that this tag in general has colors applied but they are not returned in certain cases.

rhymes · 2020-10-08T09:33:47Z

@ludwiczakpawel I'm not very familiar with how flare tags work but I came up with these two clues:

article.flare_tag always calls the FlareTag class with just the article object, without the "except tag" parameter:

forem/app/models/article.rb

Lines 312 to 314 in a943f04

    
           def flare_tag 
        
             @flare_tag ||= FlareTag.new(self).tag_hash 
        
           end

forem/app/services/flare_tag.rb

Lines 18 to 21 in a943f04

    
           def initialize(article, except_tag = nil) 
        
             @article = article.decorate 
        
             @except_tag = except_tag 
        
           end

It looks to me that if FlareTag were to be initialized with a second argument, it would send back nil even if the Tag itself has the colors assigned to it:

forem/app/services/flare_tag.rb

Lines 37 to 40 in a943f04

    
           def tag_id 
        
             tag_name, tag_id = FLARE_TAG_IDS_HASH.slice(*article.cached_tag_list_array).first 
        
             tag_id if tag_name && tag_name != except_tag 
        
           end

The clue for what might be going on in your recording is to be found in:

forem/app/views/articles/_tag_identifier.html.erb

Lines 1 to 3 in a943f04

    
           <% flare_tag = FlareTag.new(story, @tag).tag %> 
        
           <% if flare_tag %> 
        
             <span class="crayons-story__flare-tag" style="background:<%= flare_tag.bg_color_hex %>;color:<%= flare_tag.text_color_hex %>">

which is passing @tag as the "except tag".

This comes from https://github.com/forem/forem/blob/master/app/views/articles/_single_story.html.erb

I seem to recall that the first page of the feed is rendered one way, subsequent pages are rendered in a different way but I'm not sure how all that works, @joshpuetz or @nickytonline might instantly know what's going on here.

There might be a different behavior between the flare tag appearing in the first batch of articles and the following batches

akshayymahajan · 2020-10-14T15:46:57Z

I tried removing the "except tag" from the call, but it still returns null value for colors. I also tried the latest master branch locally and the JSON returned still has null values. Looks to me like this is something related to the local development environment.

ludwiczakpawel · 2020-10-14T16:04:37Z

@akshayymahajan let me give it another look tomorrow and I'll get back to you then.

ludwiczakpawel · 2020-10-15T14:08:55Z

Yea, looks like it's being broken on master as well.. So I'll go ahead and approve this PR :) Thanks for all the work you've done, it's really something I've been looking for for a quite long time so I appreciate your effort :)

…rem#10528) * Remove flare tag from title and hightlight it within the tags row * wrap flare tag # in a span with class crayons-tag__prefix * updated buildArticleHTML.js to show flare tag in the tags row * updated _single_story.html.erb to show flare tag in the tags row * fixed missing opening tag * removed crayons-story__flare-tag class and containers * added .crayons-story__flare-tag back to stories.scss for search results * added check for undefined tagList in buildArticleHTML.js

Remove flare tag from title and hightlight it within the tags row

25653cb

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 2, 2020

rhymes requested review from lisasy and ludwiczakpawel October 2, 2020 08:51

ludwiczakpawel suggested changes Oct 2, 2020

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 2, 2020

akshayymahajan added 3 commits October 2, 2020 14:44

wrap flare tag # in a span with class crayons-tag__prefix

bde7cbb

updated buildArticleHTML.js to show flare tag in the tags row

6370976

updated _single_story.html.erb to show flare tag in the tags row

7c9e724

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 2, 2020

fixed missing opening tag

9438e28

ludwiczakpawel suggested changes Oct 2, 2020

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 2, 2020

removed crayons-story__flare-tag class and containers

9a7d14b

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 2, 2020

ludwiczakpawel approved these changes Oct 2, 2020

View reviewed changes

app/assets/stylesheets/components/stories.scss Outdated Show resolved Hide resolved

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 2, 2020

lisasy approved these changes Oct 2, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 2, 2020

added .crayons-story__flare-tag back to stories.scss for search results

3a18784

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Oct 3, 2020

added check for undefined tagList in buildArticleHTML.js

cb32e51

ludwiczakpawel self-requested a review October 5, 2020 07:02

ludwiczakpawel suggested changes Oct 5, 2020

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 5, 2020

ludwiczakpawel approved these changes Oct 15, 2020

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 15, 2020

ludwiczakpawel merged commit ff918da into forem:master Oct 15, 2020

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 15, 2020

Remove flare tag from title and hightlight it within the tags row #10528

Remove flare tag from title and hightlight it within the tags row #10528

akshayymahajan commented Oct 2, 2020

ludwiczakpawel left a comment

ludwiczakpawel Oct 2, 2020

akshayymahajan Oct 2, 2020

ludwiczakpawel commented Oct 2, 2020

akshayymahajan commented Oct 2, 2020

ludwiczakpawel commented Oct 2, 2020

akshayymahajan commented Oct 2, 2020

ludwiczakpawel left a comment

ludwiczakpawel Oct 2, 2020

ludwiczakpawel Oct 2, 2020

ludwiczakpawel Oct 2, 2020

ludwiczakpawel Oct 2, 2020

ludwiczakpawel Oct 2, 2020

akshayymahajan commented Oct 2, 2020

ludwiczakpawel left a comment

lisasy left a comment

rhymes commented Oct 4, 2020

akshayymahajan commented Oct 4, 2020

ludwiczakpawel left a comment

akshayymahajan commented Oct 5, 2020

ludwiczakpawel commented Oct 5, 2020

rhymes commented Oct 8, 2020

akshayymahajan commented Oct 8, 2020

ludwiczakpawel commented Oct 8, 2020 •

edited

rhymes commented Oct 8, 2020

akshayymahajan commented Oct 14, 2020

ludwiczakpawel commented Oct 14, 2020

ludwiczakpawel commented Oct 15, 2020

Remove flare tag from title and hightlight it within the tags row #10528

Remove flare tag from title and hightlight it within the tags row #10528

Conversation

akshayymahajan commented Oct 2, 2020

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Added tests?

Added to documentation?

ludwiczakpawel left a comment

Choose a reason for hiding this comment

ludwiczakpawel Oct 2, 2020

Choose a reason for hiding this comment

akshayymahajan Oct 2, 2020

Choose a reason for hiding this comment

ludwiczakpawel commented Oct 2, 2020

akshayymahajan commented Oct 2, 2020

ludwiczakpawel commented Oct 2, 2020

akshayymahajan commented Oct 2, 2020

ludwiczakpawel left a comment

Choose a reason for hiding this comment

ludwiczakpawel Oct 2, 2020

Choose a reason for hiding this comment

ludwiczakpawel Oct 2, 2020

Choose a reason for hiding this comment

ludwiczakpawel Oct 2, 2020

Choose a reason for hiding this comment

ludwiczakpawel Oct 2, 2020

Choose a reason for hiding this comment

ludwiczakpawel Oct 2, 2020

Choose a reason for hiding this comment

akshayymahajan commented Oct 2, 2020

ludwiczakpawel left a comment

Choose a reason for hiding this comment

lisasy left a comment

Choose a reason for hiding this comment

rhymes commented Oct 4, 2020

akshayymahajan commented Oct 4, 2020

ludwiczakpawel left a comment

Choose a reason for hiding this comment

akshayymahajan commented Oct 5, 2020

ludwiczakpawel commented Oct 5, 2020

rhymes commented Oct 8, 2020

akshayymahajan commented Oct 8, 2020

ludwiczakpawel commented Oct 8, 2020 • edited

rhymes commented Oct 8, 2020

akshayymahajan commented Oct 14, 2020

ludwiczakpawel commented Oct 14, 2020

ludwiczakpawel commented Oct 15, 2020

ludwiczakpawel commented Oct 8, 2020 •

edited