Add a unified search component. #5997

zinigor · 2016-12-27T16:03:50Z

This adds a new component that will include all tabs and pass search terms into them. That will allow us to get rid of a dedicated search view and use existing ones instead.

Changes proposed in this Pull Request:

Adds searchable settings view.
Changes the composing view inside the Writing tab to respect search terms.

Testing instructions:

Try to search for something related to markdown and after-the-deadline modules.
Observe the respective configuration settings

What else to look at

Make sure that when you try to search, and there's no search term yet, settings are not empty.

zinigor · 2016-12-28T08:54:08Z

#5461

zinigor · 2016-12-30T12:11:44Z

Needs more work on Writing once it's done, but still marking it as Needs Review for general direction review.

eliorivero · 2017-01-04T12:54:11Z

Rebased after merging #6020
Conflicts fixed in writing/index.jsx and writing/composing.jsx

eliorivero · 2017-01-04T13:57:23Z

Some things I found in this branch (tested before rebasing to make sure I wasn't altering anything):

The first time you click on search the setting cards of the current tab continue to be displayed. However, after you type something and later clear the field they're not longer displayed. Either they should not be displayed to begin with or they should be displayed after clearing the field.
If you reload while you're searching, after reloading, the app ends up in search state and nothing is displayed. In current JPR when you reload you end up in search too but the list of modules is displayed so it doesn't look like there's something missing.
The definition of lists such as let list = [ markdown, atd ].map( function( m ) { is right now inconsistent since some definitions are in a top component like traffic/index.jsx and others in a sub component like writing/composing.jsx. They should be standardized and defined all in top components and when needed passed down to sub components. This could be a helper function, it uses the same code structure every time is added.

zinigor · 2017-01-18T11:56:36Z

Thanks for the detailed review, Elio! I have fixed the first problem, but I'm not sure what to do about the second. @MichaelArestad can we have your advice on the UX here? Refreshing during search is quite an edge case, what should we do about it?

MichaelArestad · 2017-01-19T19:45:19Z

@zinigor If they refresh, show the search and list everything until they exit the search or start typing.

zinigor · 2017-01-23T14:51:14Z

This is now ready for testing. Several caveats:

Scan and Antispam settings show up even if you search for Protect or SSO because we don't have descriptions for them in the API response yet. This will be handled separately in an effort to start moving settings to a module-agnostic endpoint.
The refresh problem is not handled yet, @MichaelArestad are you sure that showing everything is a good way to handle the situation? Maybe it's better to display a message that says something like: "Please enter a search term."?
The search bar now tracks user input focus, that's intentional. It shows up either if you have activated search and have #/search in the URL, or if you have navigated back, but still have focus inside the input. That solves the problem of blank search results when deleting a search term.

MichaelArestad · 2017-01-23T23:13:39Z

The refresh problem is not handled yet, @MichaelArestad are you sure that showing everything is a good way to handle the situation? Maybe it's better to display a message that says something like: "Please enter a search term."?

I am not. What other options do we have?

We could show an encouraging message to get someone to search. (I don't think we do this anywhere else, but if we do, lets do it.)

MichaelArestad · 2017-01-23T23:14:12Z

@jeffgolenski @rickybanister ideas/thoughts? ^

rickybanister · 2017-01-24T15:13:58Z

I don't think we need any placeholder text or anything. If search is focused, we should hide the current settings cards and just leave the rest of the page blank until something is typed. At most we could show a message below the search box—

Something like

Find a setting by name or keyword that then changes to 3 features matching 'writing'

Like /people

And if no results we could do something like this

eliorivero · 2017-02-01T23:07:28Z

Needs a nice rebase. Labeling as In Progress and moving it accordingly in project board.

* Passed searchTerm from Main to all settings tabs. * Added search functionality for the Writing tab. * Properly added tabs to searchable settings and added discussion search terms. * Added search terms to traffic tab. * Added search terms for Security tab. * Making sure the page doesn't disappear when a user first clicks Search. * Made sure the empty Composing section isn't shown.

Search will no longer show an empty page on clearing the term, but will rather go back to the previous page.

It will help determine if a module matches the current search. * Made the writing tab use the new search selector. * Made the traffic tab use the new found selector. * Added new search functionality to the security tab. * Added general settings to the new search.

* Also removed unused imports in Main. * Fixed several eslint warnings. * Fixed the writing settings tab.

zinigor · 2017-02-14T12:13:31Z

Rebased, now ready to test!

zinigor · 2017-02-14T14:39:03Z

This has grown too huge, let's merge it into the feature branch and move further with smaller changes.

eliorivero

@zinigor

there's a change that ended up being lost, I guess while rebasing. It's this and all its subsequent usages:
https://github.com/Automattic/jetpack/pull/5997/files#diff-b90f6848452ec5e1c5bcb466b4628d12L168

I noticed this when I clicked the Sharing nav tab, it tried to go to http://example.org/wp-admin/undefinedoptions-general.php?page=sharing
when you click the x to close the search, if the field is empty, you exit the search mode, as can be seen here
there's a jump when you clear the field. Happens when you do it by keyboard or by mouse
finally, the first time I reviewed this I mentioned that it was inconsistent that when you started searching, all setting cards were visible, but if you for some reason reloaded or followed a link straight to #search, no setting cards were visible. To make it consistent, we should never display any card while in search mode and when the field is empty. This is how the settings search in iPhone and Android works so some people will be familiar.

zinigor · 2017-02-14T16:54:34Z

@eliorivero thanks for thorough testngi! What do you say I fix number 1 and we merge, while creating issues for everything else? This PR is too heavy to rebase each time stuff gets modified in the feature branch.

eliorivero · 2017-02-14T17:02:05Z

ok cool, yeah, the first one is why I blocked this. The others can be improved later, specially 2 and 3.

zinigor · 2017-02-14T17:16:52Z

OK, done. @eliorivero can you please take a look again at the last commit and merge if it's OK?

eliorivero · 2017-02-14T21:06:07Z

_inc/client/main.jsx

@@ -174,36 +182,22 @@ const Main = React.createClass( {
 				pageComponent = <Apps siteRawUrl={ this.props.siteRawUrl } />;
 				break;
 			case '/plans':
-				pageComponent = <Plans siteRawUrl={ this.props.siteRawUrl } siteAdminUrl={ this.props.siteAdminUrl } />;
+				pageComponent = <Plans siteRawUrl={ this.props.siteRawUrl } siteAdminUrl={ this } />;


This change is not ok, should still have the props.siteAdminUrl otherwise the URL for example for "View your spam stats" ends up like http://example.tld/wp-admin/[object%20Object]admin.php?page=akismet-key-config

eliorivero

Will merge this for now. The one concern I have is that we're connecting all these middle components when it's not necessary. This introduces a slight overhead in performance and is harder to maintain so they should be removed and get props from their parent component which is already connected.

Remove unused file modules-per-tab-page.jsx. The only file using this component was removed in #5997 #### Changes proposed in this Pull Request:  * Removes _inc/client/components/module-settings/modules-per-tab-page.jsx #### Testing instructions: 1. Standing on the jetpack directory. 2. Run `grep -r AllModuleSettings *`. (The file removed by this PR exports a named export so usages of it can be greppable. 2. Confirm that no result comes up. #### Proposed changelog entry for your changes: * None needed

zinigor added General [Status] In Progress labels Dec 27, 2016

zinigor requested review from oskosk and eliorivero December 28, 2016 08:54

zinigor force-pushed the add/unified-search branch from 40e9eb8 to 31b7d6d Compare December 28, 2016 13:50

zinigor added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Dec 30, 2016

eliorivero force-pushed the add/unified-search branch from 05525f2 to cd35447 Compare January 4, 2017 12:52

eliorivero force-pushed the add/unified-search branch 3 times, most recently from 1b418dd to da24867 Compare January 4, 2017 13:37

jeherve added this to the 4.6 milestone Jan 9, 2017

eliorivero modified the milestones: Settings UI, 2/17 - February Jan 30, 2017

eliorivero added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Feb 1, 2017

zinigor force-pushed the add/unified-search branch from 5f76ab3 to a0f72ec Compare February 3, 2017 09:28

zinigor force-pushed the add/unified-search branch from c62db2b to 6a400c2 Compare February 14, 2017 11:19

zinigor added 4 commits February 14, 2017 14:37

Added search blur behaviour.

6844690

Search will no longer show an empty page on clearing the term, but will rather go back to the previous page.

Cleaned out old search component.

26315e1

* Also removed unused imports in Main. * Fixed several eslint warnings. * Fixed the writing settings tab.

zinigor force-pushed the add/unified-search branch from 6a400c2 to 26315e1 Compare February 14, 2017 12:12

zinigor added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Feb 14, 2017

zinigor added 3 commits February 14, 2017 17:20

Added a 'nothing found notification'.

febd2f8

Changed a message on empty search string.

460d15a

Removed engagement tab remains and fixed tests for GUI.

94c4f86

eliorivero suggested changes Feb 14, 2017

View reviewed changes

Added changes that were lost during rebase.

d390164

Fixed a typo.

d1233fc

eliorivero reviewed Feb 14, 2017

View reviewed changes

Added back properties lost in rebase.

db8b432

eliorivero approved these changes Feb 14, 2017

View reviewed changes

eliorivero merged commit 6e8c6f2 into feature/settings-overhaul Feb 14, 2017

eliorivero deleted the add/unified-search branch February 14, 2017 21:45

eliorivero removed the [Status] Needs Review This PR is ready for review. label Feb 14, 2017

eliorivero mentioned this pull request Feb 14, 2017

Settings UI - Search: don't connect sub components to global state tree #6417

Closed

oskosk mentioned this pull request Nov 16, 2018

Admin Page: Remove unused file modules-per-tab-page.jsx #10644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a unified search component. #5997

Add a unified search component. #5997

zinigor commented Dec 27, 2016 •

edited

Loading

zinigor commented Dec 28, 2016

zinigor commented Dec 30, 2016

eliorivero commented Jan 4, 2017

eliorivero commented Jan 4, 2017

zinigor commented Jan 18, 2017

MichaelArestad commented Jan 19, 2017

zinigor commented Jan 23, 2017 •

edited

Loading

MichaelArestad commented Jan 23, 2017

MichaelArestad commented Jan 23, 2017

rickybanister commented Jan 24, 2017

eliorivero commented Feb 1, 2017

zinigor commented Feb 14, 2017

zinigor commented Feb 14, 2017

eliorivero left a comment •

edited

Loading

zinigor commented Feb 14, 2017

eliorivero commented Feb 14, 2017

zinigor commented Feb 14, 2017

eliorivero Feb 14, 2017

eliorivero left a comment

Add a unified search component. #5997

Add a unified search component. #5997

Conversation

zinigor commented Dec 27, 2016 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

What else to look at

zinigor commented Dec 28, 2016

zinigor commented Dec 30, 2016

eliorivero commented Jan 4, 2017

eliorivero commented Jan 4, 2017

zinigor commented Jan 18, 2017

MichaelArestad commented Jan 19, 2017

zinigor commented Jan 23, 2017 • edited Loading

MichaelArestad commented Jan 23, 2017

MichaelArestad commented Jan 23, 2017

rickybanister commented Jan 24, 2017

eliorivero commented Feb 1, 2017

zinigor commented Feb 14, 2017

zinigor commented Feb 14, 2017

eliorivero left a comment • edited Loading

Choose a reason for hiding this comment

zinigor commented Feb 14, 2017

eliorivero commented Feb 14, 2017

zinigor commented Feb 14, 2017

eliorivero Feb 14, 2017

Choose a reason for hiding this comment

eliorivero left a comment

Choose a reason for hiding this comment

zinigor commented Dec 27, 2016 •

edited

Loading

zinigor commented Jan 23, 2017 •

edited

Loading

eliorivero left a comment •

edited

Loading