Fixes #29307 - Add patternfly4 search to new CV page #8745

johnpmitsch · 2020-06-03T15:40:49Z

Adds new search at /labs/content_views

To-do:

tests
fix pf4 search icon to match mockups UPDATE: open question to UX
fix issue where you can't click on the first item in dropdown until you mouse over another item and back to it.
In the autocomplete there is an "operators" line in italics on the traditional CV page that needs to be added
UPDATE: this is an existing issue in pf3 pages as well, I filed here https://projects.theforeman.org/issues/30071 and added to the feature tracker to look later.
Add tests for Search component (a nice-to-have, but will make the components easier to move to Foreman)

For this addition, we need to support pf3 and pf4 search. I originally tried to see what would happen if I updated the search to pf4, but it looks really out of place in the context of a pf3 page.

I updated the TypeAhead component to support both pf3 and pf4 so it could share the autocomplete functionality coming from the downshift package. The Search component now takes a patternfly4 prop and defaults to pf3 if not set.

I have some duplicated file names where the folder determines if the component is pf3 and pf4. This made sense to me, but I am open to suggestions. React (for better or worse) doesn't have a lot of opinions about file naming or conventions, and I don't see any strong opinions out there against this pattern, so it is really whatever we are comfortable with.

theforeman-bot · 2020-06-03T15:40:56Z

Issues: #29307

johnpmitsch · 2020-06-03T15:45:49Z

@jeremylenz I'm opening this up as a draft, I put the remaining tasks in the PR message. If you have a chance to look, I would love to hear any feedback on the overall approach (especially before I write tests)

I moved the TypeAhead component out of the move_to_pf folder, that is in a separate commit to keep the changes separate. The reasoning for this is (from the commit message):

I would like to get away from the move_to_* folders. Our intent can still be to move the components to patternfly or foreman, but we don't necessarily have to call our folders that. These "temporary" folders have been here for years and we should take steps to be realistic about what we can and can't move and not organize our folders based on these assumptions, which makes things harder to find.

webpack/utils/useEventListener.js

jeremylenz

Thanks @johnpmitsch! A few initial comments below but I didn't get to play around too much, because I'm getting a console error when I search for "label = xxx" and then execute the search:

webcomponents-bundle.js:92 Uncaught TypeError: Cannot convert undefined or null to object
    at Function.getOwnPropertyNames (<anonymous>)
    at Function.../vendor-core/node_modules/@webcomponents/webcomponentsjs/webcomponents-bundle.js.Object.keys (webcomponents-bundle.js:92)
    at tableDataGenerator.js:124
    at Array.forEach (<anonymous>)
    at tableDataGenerator (tableDataGenerator.js:120)
    at ContentViewsTable.js:20
    at commitHookEffectList (react-dom.development.js:19986)
    at commitPassiveHookEffects (react-dom.development.js:20016)
    at HTMLUnknownElement.callCallback (react-dom.development.js:347)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:397)
../vendor-core/node_modules/@webcomponents/webcomponentsjs/webcomponents-bundle.js.Object.keys @ webcomponents-bundle.js:92
(anonymous) @ tableDataGenerator.js:123
tableDataGenerator @ tableDataGenerator.js:120
(anonymous) @ ContentViewsTable.js:19
commitHookEffectList @ react-dom.development.js:19986
commitPassiveHookEffects @ react-dom.development.js:20016
callCallback @ react-dom.development.js:347
invokeGuardedCallbackDev @ react-dom.development.js:397
invokeGuardedCallback @ react-dom.development.js:454
flushPassiveEffectsImpl @ react-dom.development.js:22868
unstable_runWithPriority @ scheduler.development.js:643
runWithPriority$2 @ react-dom.development.js:11305
flushPassiveEffects @ react-dom.development.js:22841
(anonymous) @ react-dom.development.js:22419
scheduler_flushTaskAtPriority_Normal @ scheduler.development.js:436
flushTask @ scheduler.development.js:482
flushWork @ scheduler.development.js:607
performWorkUntilDeadline @ scheduler.development.js:231
react_devtools_backend.js:6 The above error occurred in the <ContentViewTable> component:
    in ContentViewTable (created by ContentViewsPage)
    in ContentViewsPage (created by Context.Consumer)
    in withRouter(ContentViewsPage) (created by Headers)
    in Headers (created by Context.Consumer)
    in Route (created by _default)
    in div (created by _default)
    in _default (created by Application)
    in Router (created by BrowserRouter)
    in BrowserRouter (created by Application)
    in Application (created by ConnectFunction)
    in ConnectFunction (created by I18nProviderWrapper(Connect(Application)))
    in IntlProvider (created by I18nProviderWrapper(Connect(Application)))
    in I18nProviderWrapper(Connect(Application)) (created by StoreProvider(I18nProviderWrapper(Connect(Application))))
    in Provider (created by StoreProvider(I18nProviderWrapper(Connect(Application))))
    in StoreProvider(I18nProviderWrapper(Connect(Application))) (created by DataProvider(StoreProvider(I18nProviderWrapper(Connect(Application)))))
    in DataProvider(StoreProvider(I18nProviderWrapper(Connect(Application))))

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

webpack/utils/useEventListener.js

jeremylenz · 2020-06-05T02:53:43Z

webpack/components/Table/TableWrapper.js

 import { usePaginationOptions, useForemanSettings } from 'foremanReact/Root/Context/ForemanContext';

-import EmptyStateMessage from './EmptyStateMessage';
-import Loading from './Loading';
+import MainTable from './MainTable';


👍 for breaking this out into its own file, makes it feel a little cleaner.

webpack/components/TypeAhead/TypeAhead.js

webpack/components/TypeAhead/pf4Search/TypeAheadInput.js

johnpmitsch · 2020-06-09T00:31:56Z

fix pf4 search icon to match mockups

@MariSvirik Regarding the seach icon - from what I can tell the way our search and RH cloud search function is different. They search automatically and the magnifying glass is not a button, the search happens as you type. For instance:

With this search, I didn't click anything or hit enter to get this result, the search happens as you type.

Whereas for Foreman/Katello search, you have to click enter or click the search icon. So it seems like keeping it as a button is better, but I am wondering what you think is best. Right now I plan to copy the example in https://patternfly-react.surge.sh/documentation/react/components/toolbar but let me know if I should do something different!

johnpmitsch · 2020-06-09T13:04:11Z

Thanks @johnpmitsch! A few initial comments below but I didn't get to play around too much, because I'm getting a console error when I search for "label = xxx" and then execute the search:

@jeremylenz This is fixed now!

johnpmitsch · 2020-06-09T14:38:20Z

Add tests for Search component (a nice-to-have, but will make the components easier to move to Foreman)

I added one to make sure autocomplete shows, hopefully we can add more, but it is a start!

johnpmitsch · 2020-06-09T17:04:03Z

fix issue where you can't click on the first item in dropdown until you mouse over another item and back to it.

Fixed this now, thanks @jeremylenz!

Btw https://projects.theforeman.org/issues/29907 should also be fixed in this PR

jeremylenz

Thanks for moving it along!

Works mostly as expected, with a couple issues remaining. And I'm not seeing any more page flashes, so I think we're good with 29907.

When I change the per-page setting, the table reloads but executes a blank search, instead of the current active search. I would expect it to either clear out the search box, or re-execute the existing search.
If I click the X button, it displays the loading spinner and re-does the "search" for all items. As a user, I would expect clicking the X to be instant and not require waiting. At the least, we should store the "all items" search in memory (memoize it) so it doesn't have to make an API call every time. As to how, whatever makes the most sense to you -- Redux, useMemo, etc. Can be a follow-up PR.
A search for "not composite" is suggested when I type "not", but results in an error. (Btw, the error empty state looks great 😄)

jeremylenz · 2020-06-09T19:39:36Z

webpack/components/TypeAhead/TypeAhead.js

+      inputValue: this.props.initialInputValue,
+    };
+
+    this.handleStateChange = this.handleStateChange.bind(this);


If you do

handleStateChange = ({inputValue}) => { ...

then you don't have to do the bind in the constructor here :)

jeremylenz · 2020-06-09T19:43:34Z

webpack/components/TypeAhead/pf3Search/TypeAheadSearch.js

+e, isOpen, activeItems, highlightedIndex,
+                                          selectItem, userInputValue, onSearch,
+)}


Did eslint take a lunch break or something? 😆

This looks fine on my local system.. weird

jeremylenz · 2020-06-09T20:05:13Z

webpack/components/TypeAhead/pf4Search/TypeAheadSearch.js

+e, isOpen, activeItems, highlightedIndex,
+                            selectItem, userInputValue, onSearch,


please format

jeremylenz · 2020-06-09T20:31:56Z

webpack/components/TypeAhead/pf4Search/TypeAheadInput.js

+  return (
+    <TextInput
+      {...downshiftProps}
+      ref={inputRef}


👍 for switching to the useRef and non-function form of the prop. I was also going to suggest dropping ref in favor of the innerRef prop provided by PF4, but both ref and innerRef seem to put the event listener on the <input> element and both work fine. So I guess either is fine.

webpack/scenes/ContentViews/Table/tableDataGenerator.js

webpack/scenes/ContentViews/__tests__/contentViewPage.test.js

johnpmitsch · 2020-06-10T13:32:23Z

A search for "not composite" is suggested when I type "not", but results in an error. (Btw, the error empty state looks great smile)

Thanks, and ya I saw that, it looks pre-existing and on the server side so I filed here https://projects.theforeman.org/issues/30079

It will definitely is more of a concern with autosearching on typing. Speaking with UX and others yesterday about it, it sounds like we would like to try out automatic searching on typing with an option to disable, and we can do more testing of this with a nightly box with a lot of content views to see how it performs.

So I'll update the PR with this as well as the other updates from review!

johnpmitsch · 2020-06-16T02:08:32Z

@jeremylenz was able to update this and also include autosearch on typing, I'll follow up with a comment on how that works. I would like to add some more tests to Search, but otherwise it is ready for re-review 👍

When I change the per-page setting, the table reloads but executes a blank search, instead of the current active search. I would expect it to either clear out the search box, or re-execute the existing search.

Fixed!

If I click the X button, it displays the loading spinner and re-does the "search" for all items. As a user, I would expect clicking the X to be instant and not require waiting. At the least, we should store the "all items" search in memory (memoize it) so it doesn't have to make an API call every time. As to how, whatever makes the most sense to you -- Redux, useMemo, etc. Can be a follow-up PR.

This isn't how the existing pages work (they also make an API call when you clear the search) and afaik we didn't plan to change this behavior in this feature. If you want to file it and propose it for the future it could be a good addition, but I don't consider within scope of the new CV page.

johnpmitsch · 2020-06-16T02:20:15Z

@MariSvirik @Rohoover @jeremylenz
I added auto search typing similar to cloud.redhat.com. This is what it looks like:

There are two new settings to disable it and also to configure the delay before searching. I defaulted to 500ms - 1 second felt too unresponsive and anything under 500ms sent too many requests if you typed slow. The settings are Automatic Search Typing Delay and Automatic Search while Typing seen below. I could see those with large amounts of content wanting higher delays or to disable the automatic searching completely.

If you disable the automatic search, it functions the same as our existing search, but with pf4 components:

Let me know if you have any suggestions or questions!

johnpmitsch · 2020-06-16T02:37:18Z

@MariSvirik @Rohoover I forgot to show this, here is the empty search results and error state pages, I mostly followed patternfly's design guidelines, but let me know if anything should be tweaked :)

(this should only be seen if there is a bug or the server is down)

MariSvirik · 2020-06-16T13:32:42Z

@johnpmitsch @Rohoover
Regarding empty states, I proposed this phrasing (consulted with Margot):

No matching content views found (RHtext medium, 20)
Try changing your search settings. (RHtext regular, 16)

Unable to connect
There was an error retrieving data from the server. Check your connection and try again.

johnpmitsch · 2020-06-16T16:11:03Z

@johnpmitsch @Rohoover
Regarding empty states, I proposed this phrasing (consulted with Margot):

No matching content views found (RHtext medium, 20)
Try changing your search settings. (RHtext regular, 16)

Unable to connect
There was an error retrieving data from the server. Check your connection and try again.

@MariSvirik updated the text! I have to ask about the fonts, I think they are set in Foreman and would need to be changed there.

Here are the new pages:

johnpmitsch · 2020-06-16T16:19:10Z

I would like to add some more tests to Search,

These are now added as well

MariSvirik · 2020-06-16T19:36:28Z

@johnpmitsch just a slight note about capitalization.
The name of the main page is 'Content Views' (both words capitalized) because it is also an item in the Navigation where everything should be in title case (the rule from Insights). We should use 'Content Views' when referring to the page itself. When you refer to a content view in general (not the page as such) you should use sentence case.
In our empty state, just a first letter of the first word of the sentence (n) should be capitalized: 'No matching content views found'

Thanks for doing such a good job 🥇

johnpmitsch · 2020-06-17T13:41:35Z

In our empty state, just a first letter of the first word of the sentence (n) should be capitalized: 'No matching content views found'

thanks, I updated this!

jeremylenz

Tested and works great! Love the debounced type-ahead search, though I really do miss the X button in the input. I'm guessing that's already decided on?

Comments below

webpack/components/Search/Search.js

jeremylenz · 2020-06-18T18:41:15Z

webpack/components/Search/Search.js

+    // Prevent empty search when page first loads, but search when input is cleared after that
+    this.setState({ autoSearchOnEmpty: true });


Why not just remove this.onInputUpdate() from componentDidMount? I'm guessing it's because you need to do part of this function on mount, but not all of it. If that's the case, could this function be split into parts to remove the need to track autoSearchOnEmpty?

Removing it seems to work fine, I'm not sure why it was there in the first place 🤔 I checked RH repos and subscriptions page too and they work fine. Please double check me though!

I removed autoSearchOnEmpty from the state and also removed the 2 places where it's read and written, and everything seems to work fine. I'd say it's safe to remove

webpack/components/Search/Search.js

webpack/components/Search/__tests__/search.test.js

jeremylenz · 2020-06-18T19:08:52Z

webpack/components/Search/index.js

+import { connect } from 'react-redux';
+import * as settingActions from 'foremanReact/components/Settings/SettingsActions';
+import { selectSettings } from '../../scenes/Settings/SettingsSelectors';
+import Search from './Search';


Oh. This answers my question from above. Seems like <Search> is mostly moved code rather than new code. In that case, take your pick what to address 😄

ya its mostly moved out of the index.js. I'll keep it as a class component but maybe there are a couple places i can clean up like removing .bind(this) 🧹

+1 to removing the .binds 😄

jeremylenz · 2020-06-18T19:25:42Z

app/models/setting/content.rb

+      self.set('autosearch_while_typing', N_('For pages that support it, automatically perform search while typing in search input.'),
+               true, N_('Automatic Search while Typing')),
+      self.set('autosearch_delay', N_('Delay in milliseconds before performing search for automatic searching.'),
+               500, N_('Automatic Search Typing Delay'))


This shows up in Settings before the other one:

Automatic Search Typing Delay -- | -- | -- Automatic Search while Typing

It appears to be alphabetical. Would make more sense if the main setting showed first, followed by the delay setting. Would naming them "Search as you Type" and "Search as you Type Delay" work?

@MariSvirik any thoughts on the naming of these settings?

decided on Autosearch and Autosearch delay (May not be one word)

According to the content team, it should be one word (not even hyphened) in order to be consistent with the way IBM hyphens their words.

johnpmitsch · 2020-06-19T00:30:48Z

I really do miss the X button in the input. I'm guessing that's already decided on?

@MariSvirik any thoughts on this? Should we keep the X clear button for the auto search as well?

MariSvirik · 2020-06-19T15:03:42Z

Decision after meeting: add X button to the right edge of the searchbar to provide the easy way for the query deletion.

I would like to get away from the move_to_* folders. Our intent can still be to move the components to patternfly or foreman, but we don't necessarily have to call our folders that. These "temporary" folders have been here for years and we should take steps to be realistic about what we can and can't move and not organize our folders based on these assumptions, which makes things harder to find.

johnpmitsch · 2020-06-19T18:27:51Z

@jeremylenz @MariSvirik Updated the setting names!

(@jeremylenz use Setting::Content.destroy_all! in the rails console and restart the server to update them)

Also added an "X" when autosearch is enabled and there is input

~~I still have to update the one test discussed here and fix some linting, but pushing up what I have so far for feedback :)~~
EDIT: updated!

johnpmitsch · 2020-06-22T15:14:30Z

[test katello]

jeremylenz

looking good @johnpmitsch! Just a few comments below

app/models/setting/content.rb

webpack/components/Search/Search.js

jeremylenz · 2020-06-23T19:14:20Z

webpack/components/Search/Search.js

+    // Prevent empty search when page first loads, but search when input is cleared after that
+    this.setState({ autoSearchOnEmpty: true });


I removed autoSearchOnEmpty from the state and also removed the 2 places where it's read and written, and everything seems to work fine. I'd say it's safe to remove

jeremylenz · 2020-06-23T19:16:09Z

webpack/components/Search/index.js

+import { connect } from 'react-redux';
+import * as settingActions from 'foremanReact/components/Settings/SettingsActions';
+import { selectSettings } from '../../scenes/Settings/SettingsSelectors';
+import Search from './Search';


+1 to removing the .binds 😄

jeremylenz · 2020-06-23T19:17:22Z

webpack/components/TypeAhead/pf3Search/TypeAheadSearch.js

+e, isOpen, activeItems, highlightedIndex,
+                                          selectItem, userInputValue, onSearch,
+)}


This looks fine on my local system.. weird

webpack/global_test_setup.js

webpack/scenes/ContentViews/Table/ContentViewsTable.js

johnpmitsch · 2020-06-24T14:54:45Z

@jeremylenz updated!

jeremylenz

Changes look good, thanks @johnpmitsch !

tbrisker · 2020-07-28T08:04:21Z

app/models/setting/content.rb

+      self.set('autosearch_while_typing', N_('For pages that support it, automatically perform search while typing in search input.'),
+               true, N_('Autosearch')),
+      self.set('autosearch_delay', N_('If Autosearch is enabled, delay in milliseconds before executing searches while typing.'),
+               500, N_('Autosearch delay'))


TBH i don't think these should be configurable settings. The delay should be set to a reasonable default and the auto search should only be enabled on pages where it can return result in a timely fashion without hammering the server with requests. (i.e. <100 ms to get results for a full db)

@tbrisker I am planning on running some production tests to see how well the autosearch performs on a servers with a lot of Content Views. I think that will allow us to make better decisions here on whether A) Autosearch should be enabled by default and B) what the delay should be. It's possible that we don't enable autosearch by default if it is not performant.

I agree we could do without the delay setting, but it would be nice to see how things perform in production first. I'm hesitant to remove it now because of how experimental this feature is. Does that sound ok to you?

My opinion is that since these settings only affect an experimental page while cluttering the settings page for all users, my preference would be to drop them completely and try to go with "[hopefully] sane default" first and only add a setting if needed in the future and we can't settle on a default that works for all users.

theforeman-bot added Needs testing Not yet reviewed labels Jun 3, 2020

johnpmitsch commented Jun 3, 2020

View reviewed changes

webpack/utils/useEventListener.js Show resolved Hide resolved

johnpmitsch force-pushed the cvsearch branch from 9a7f79f to 1ddea6e Compare June 3, 2020 16:06

jeremylenz reviewed Jun 5, 2020

View reviewed changes

johnpmitsch force-pushed the cvsearch branch 3 times, most recently from 489af1c to abacec2 Compare June 9, 2020 12:21

johnpmitsch marked this pull request as ready for review June 9, 2020 12:22

johnpmitsch force-pushed the cvsearch branch 2 times, most recently from f26b591 to 0b8c753 Compare June 9, 2020 14:35

johnpmitsch force-pushed the cvsearch branch 2 times, most recently from 0627a55 to 5f3a09a Compare June 9, 2020 16:50

jeremylenz requested changes Jun 9, 2020

View reviewed changes

johnpmitsch force-pushed the cvsearch branch from 6de659e to 260aecb Compare June 16, 2020 02:03

johnpmitsch force-pushed the cvsearch branch 2 times, most recently from eba7ab1 to a77da06 Compare June 16, 2020 02:33

johnpmitsch closed this Jun 16, 2020

johnpmitsch reopened this Jun 16, 2020

johnpmitsch force-pushed the cvsearch branch from db91a49 to 75fbcb0 Compare June 16, 2020 16:08

johnpmitsch force-pushed the cvsearch branch from 75fbcb0 to 752c8ef Compare June 17, 2020 13:41

jeremylenz reviewed Jun 18, 2020

View reviewed changes

John Mitsch added 5 commits June 19, 2020 17:43

Fixes #29307 - Add patternfly4 search to new CV page

85f49e4

Refs #29307 - Updated tests

dddb12d

Refs #29307 - Added autosearch and more test updates

1c01d51

Refs #29307 - Adds more Search tests

e79cf41

johnpmitsch force-pushed the cvsearch branch from 752c8ef to beeef25 Compare June 19, 2020 18:25

Refs #29307 - PR updates

cf14a58

johnpmitsch force-pushed the cvsearch branch from beeef25 to cf14a58 Compare June 19, 2020 20:10

johnpmitsch requested a review from jeremylenz June 19, 2020 20:10

jeremylenz reviewed Jun 23, 2020

View reviewed changes

Refs #29307 - more PR updates

9aff2d2

jeremylenz approved these changes Jun 24, 2020

View reviewed changes

johnpmitsch merged commit 8f59266 into Katello:master Jun 24, 2020

johnpmitsch deleted the cvsearch branch June 24, 2020 19:11

tbrisker reviewed Jul 28, 2020

View reviewed changes

		e, isOpen, activeItems, highlightedIndex,
		selectItem, userInputValue, onSearch,

		// Prevent empty search when page first loads, but search when input is cleared after that
		this.setState({ autoSearchOnEmpty: true });

Fixes #29307 - Add patternfly4 search to new CV page #8745

Fixes #29307 - Add patternfly4 search to new CV page #8745

Conversation

johnpmitsch commented Jun 3, 2020 • edited Loading

theforeman-bot commented Jun 3, 2020

johnpmitsch commented Jun 3, 2020

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Jun 9, 2020

johnpmitsch commented Jun 9, 2020

johnpmitsch commented Jun 9, 2020

johnpmitsch commented Jun 9, 2020

jeremylenz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Jun 10, 2020

johnpmitsch commented Jun 16, 2020

johnpmitsch commented Jun 16, 2020 • edited Loading

johnpmitsch commented Jun 16, 2020

MariSvirik commented Jun 16, 2020 • edited Loading

johnpmitsch commented Jun 16, 2020

johnpmitsch commented Jun 16, 2020

MariSvirik commented Jun 16, 2020 • edited Loading

johnpmitsch commented Jun 17, 2020

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Jun 19, 2020

MariSvirik commented Jun 19, 2020

johnpmitsch commented Jun 19, 2020 • edited Loading

johnpmitsch commented Jun 22, 2020

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Jun 24, 2020

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Jun 3, 2020 •

edited

Loading

jeremylenz left a comment •

edited

Loading

johnpmitsch commented Jun 16, 2020 •

edited

Loading

MariSvirik commented Jun 16, 2020 •

edited

Loading

MariSvirik commented Jun 16, 2020 •

edited

Loading

johnpmitsch commented Jun 19, 2020 •

edited

Loading