Task/WP-72: Highlight matching search terms

[asimregmi]

Overview

PR FP-319 introduced the job history search feature. This PR achieves highlighting/bolding of the matching search terms/ query in the Job History Search.

Related

• WP-72
• FP-319

Changes

• Created a new component HighlightSearchTerm that bolds the matching queries
• Updated new props in InfiniteScrollTable component
• Added a new logic in InfiniteScrollTable.jsx for rendering the HighlightSearchTerm component

Testing

1. Go to the History page in the portal
2. Type query in the searchbar and confirm if the queries are bold
3. When no matching term is found in the Job Name and/or Output Location, it will only bold the 'View Details' button since the query is inside the View Details (the query inside the View Details Modal isn't bold)

UI

Notes

[codecov]

Codecov Report

Merging #873 (373aa9d) into main (31af29e) will increase coverage by 0.02%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##             main     #873      +/-   ##
==========================================
+ Coverage   63.34%   63.36%   +0.02%     
==========================================
  Files         428      429       +1     
  Lines       12250    12271      +21     
  Branches     2521     2532      +11     
==========================================
+ Hits         7760     7776      +16     
- Misses       4284     4288       +4     
- Partials      206      207       +1     

Flag	Coverage Δ
javascript	69.73% <78.26%> (+0.02%)	⬆️
unittests	56.99% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...common/HighlightSearchTerm/HighlightSearchTerm.jsx	100.00% <100.00%> (ø)
...common/InfiniteScrollTable/InfiniteScrollTable.jsx	97.29% <ø> (ø)
client/src/components/Jobs/Jobs.jsx	85.50% <44.44%> (-6.43%)	⬇️

[wesleyboar]

New stylesheets for React components should avoid SASS, because it is global and offers limited benefits¹.

Prefer a CSS Module to global CSS.

I.E. Rename to HighlightSearchTerm.scss.module.css and update how class is set.

Footnotes

1. The benefits of SASS, for now, are only @mixin's. Native CSS supports variables and nesting. ↩

[wesleyboar]

I approve the rendered UI.

Regarding arguably-redundant font-weight: bold CSS, read my comment about it.

[shayanaijaz]

This solution works pretty well. I did have some concerns over code structuring and the new component that was created.

• Whenever we create a new component in _common the goal is to have the component be as generic as possible so it can easily plug in anywhere we want to use it with little change/configuration. With the way HighlightSearchTerm is coded it is very specific to the job search functionality. If we wanted to add search term highlighting into another component in the future (e.g data files) then it would require a bit of rework to make it happen.

• In a table each column type can have a different cell rendering structure which is taken care of in HighlightSearchTerm using if statements. The issue I see is that cell rendering structures for these columns are already defined in Jobs.jsx when we setup the columns so redefining those with slightly different html (adding bold tags and class) seems a little redundant to me. I think I would better prefer a solution that maybe uses that column definition in Jobs.jsx and for each column that needs search term highlighting we can add the HighlightSearchTerm component.

@chandra-tacc @rstijerina I'd be curious to know what y'all think

[asimregmi]

@shayanaijaz I agree. I changed the implementation of HighlightSearchTerm component to make it more general and reusable. This new component is now being used by Jobs.jsx instead of InfiniteScrollTable.jsx unlike previous solution.

[chandra-tacc]

Great work here. Looks good.
Please see if the additional feedback can be addressed to make this even better.

[chandra-tacc]

This look great.
Minor suggestions to help improve perf.

1. Instantiate a regex and use it for split and comparison in line 9. Doing toLowerCase on searchTerm for each part is unnecessary. You have the regex defined for it already. Regex works better on shorter strings which is the split part.
2. If searchTerm is not available, return immediately.

I tried to see if just string replace with regex would work instead of split and replace, I did not get one working. But, may be you can try to see if that feasible?

suggestion code:

const highlightParts = () => {
    if (!searchTerm) {
      return content;
    }
    const searchTermRegex = new RegExp(`(${searchTerm})`, 'gi');
    const parts = content.split(searchTermRegex);
    return parts.map((part, i) =>
    part.match(searchTermRegex) ? (
        <b className={styles['highlight']} key={i}>
          {part}
        </b>
      ) : (
        part
      )
    );
  };

[asimregmi]

I changed logic to use match instead of toLowerCase. Tried the other suggestion to use .replace() function instead but couldn't get it to work properly. I am getting [object] [object' inside the string that needs to be highlighted. Is it because .replace() isn't good for returning the <b>...</b> and only good for string? Let me know.

[chandra-tacc]

For common components, can you please add unit tests. This helps to learn the behavior for usages and not break existing use cases.

[asimregmi]

Fixed by adding test cases for HighlightSearchTerm. Thank you.

[chandra-tacc]

LGTM. Thanks for adding tests and addressing comments.

[shayanaijaz]

Just a small comment to fix

[shayanaijaz]

LGTM! Good work