Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the new search - Closes #1774 #1813 #1864

Merged
merged 43 commits into from Apr 15, 2019

Conversation

Projects
None yet
5 participants
@osvaldovega
Copy link
Contributor

commented Mar 26, 2019

What issue have I solved?

-- #1774
-- #1813

How have I implemented/fixed it?

This PR includes the new design of the search bar component what involve the following changes.

  • remove the old search bar from the center of the top bar and move it next to the user details in the top bar using an search icon.
  • now will be display as a dropdown where the user is able to type anything to search about.
  • in the search results we will have 3 categories, delegates, regular accounts and transactions.
  • The search could should 1 or 2 of these categories based on what the user is looking for.
  • if something is found during the search the user is able to click the row to go to the details page.

NOTE: The search logic for this component needs to be improve to provide the information need as the mockups, so right now some mockups show so information that right now we dont have.

How has this been tested?

For test this component, the user needs to login into the application and then do a click on the search icon on the topBar (final element at the right).

Then type something to search for like and account address, delegate or transactions ID.
The result information will be display bellow.

NOTE: Please take in consideration that based on the actual search logic it is not possible to present or view this component as the new design mockups, once the search logic be improve the user will be able to see the results as the mock ups, for now it is not possible. The improvement will be implemented in a different PR.

Review checklist

osvaldovega added some commits Mar 21, 2019

@osvaldovega osvaldovega self-assigned this Mar 26, 2019

@osvaldovega osvaldovega requested a review from michaeltomasik Mar 26, 2019

osvaldovega added some commits Mar 28, 2019

@osvaldovega osvaldovega requested a review from massao Apr 1, 2019

@osvaldovega osvaldovega changed the base branch from 1.15.0 to 1.14.0 Apr 1, 2019

@osvaldovega osvaldovega changed the base branch from 1.14.0 to 1.15.0 Apr 1, 2019

@osvaldovega osvaldovega added this to the Version 1.15.0 milestone Apr 1, 2019

@osvaldovega osvaldovega changed the title Implement the new search - Closes #1774 Implement the new search - Closes #1774 #1813 Apr 1, 2019

Show resolved Hide resolved src/actions/search.js Outdated
Show resolved Hide resolved src/actions/search.test.js Outdated
Show resolved Hide resolved src/components/app/variablesV2.css Outdated
Show resolved Hide resolved src/components/searchBarV2/accounts.js Outdated
Show resolved Hide resolved src/components/searchBarV2/accountsAndDeletegates.css Outdated
Show resolved Hide resolved src/components/searchBarV2/transactions.css Outdated
Show resolved Hide resolved src/components/searchBarV2/transactions.css Outdated
Show resolved Hide resolved src/components/searchBarV2/transactions.js Outdated
Show resolved Hide resolved test/constants/selectors.js Outdated
Show resolved Hide resolved test/cypress/e2e/search.spec.js Outdated

osvaldovega added some commits Apr 1, 2019

@slaweet slaweet removed this from the Version 1.15.0 milestone Apr 2, 2019

osvaldovega added some commits Apr 3, 2019

Show resolved Hide resolved src/components/app/variablesV2.css Outdated
Show resolved Hide resolved src/components/searchBarV2/accountsAndDeletegates.css Outdated
Show resolved Hide resolved src/components/searchBarV2/accountsAndDeletegates.css Outdated
Show resolved Hide resolved src/components/searchBarV2/searchBar.css Outdated
Show resolved Hide resolved src/components/searchBarV2/searchBar.js Outdated
Show resolved Hide resolved src/components/topBar/topBar.test.js Outdated
Show resolved Hide resolved test/cypress/e2e/activity.spec.js Outdated
Show resolved Hide resolved test/cypress/e2e/search.spec.js Outdated
@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@osvaldovega Why some e2e are skipped?

@osvaldovega

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@Efefefef so far there are no skipped E2E tests the ones that I had before were already fix, so at this point all E2E related to this PR are working and not skipped

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@osvaldovega true, my bad.

@Efefefef
Copy link
Contributor

left a comment

🐛 Transfer tx: Message label instead of Amount label
🐛 Vote tx: Message label and amount value - incorrect
🐛 Second ph: Message label and amount value - incorrect
🐛 Delegate reg: Message label and amount value - incorrect
🐛 Delegate: Vote weight instead of Balance
🐛 Address of delegate acc: Doesn't show delegate name, rank
🐛 Placeholder contains 'message' mentioning, in fact, it doesn't work

@slaweet

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

🐛 Delegate: Vote weight instead of Balance

This is actually ok. I forgot to mention it in specs, but it is an exception from the design, as Balance requires additional API request.

@osvaldovega

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Hi @Efefefef about your review I fixed the labels as you mentioned, however as I mentioned in the PR note in the top, right now the search bar it is not able to see it as the mockups because the search it is not retrieving some information that it is need it for work properly and display the information.

In other words, we right now do not have message label column because at this point it is not possible to search by message, so all those title/label related to message are not showing the information that you see in the mock ups so the first thing is the at this point it is not possible search or display something related to message and second as @slaweet mentioned.

This is actually ok. I forgot to mention it in specs, but it is an exception from the design, as Balance requires additional API request.

So I just update the PR based on your review, please check it again and in case you have any doubt just let me know and we can check it with Vit.

By the way the idea is that in another ticket we need to improve the search function that retrieve the information to be possible to search any information like in the mockups

@osvaldovega osvaldovega requested a review from Efefefef Apr 11, 2019

osvaldovega added some commits Apr 11, 2019

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

Still reproducible
🐛 Second ph: Message label and amount value - should be fee
🐛 Delegate reg: Message label and amount value - should be fee

About the message: sorry for not being clear enough, I meant that the placeholder string should be changed, not the functionality.
As discussed with Vit placeholder value should be Search for Address, Transaction ID or Delegate name

osvaldovega added some commits Apr 12, 2019

@Efefefef
Copy link
Contributor

left a comment

@osvaldovega The feature is done.
Now please check the comments and fix e2e tests

describe('Search', () => {
const testnetTransaction = '755251579479131174';
const mainnetTransaction = '881002485778658401';
const testnetTransactionId = '6676752260506338126';

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 12, 2019

Contributor

How this transaction is different from the one 2 lines above? Do you need a second one?

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 12, 2019

Author Contributor

fixed

expect(getSearchesObjFromLS()[0].id).to.equal(accountsAddress);
expect(getSearchesObjFromLS()[0].searchTerm).to.equal(accountsAddress);
});
cy.wait('@requestDelegate');

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 12, 2019

Contributor

The function name describes the purpose of checking if we are at the account page. For that, we are searching for an element specific to that page. Putting here the search bar elements you are not testing anything

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 12, 2019

Author Contributor

fixed

Show resolved Hide resolved test/cypress/e2e/help.spec.js Outdated
expect(getSearchesObjFromLS()[0].id).to.equal(delegateId);
expect(getSearchesObjFromLS()[0].searchTerm).to.equal(delegateName);
});
cy.get(ss.searchTransactionRow).find(ss.searchTransactionRowId).should('have.text', transactionId);

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 12, 2019

Contributor

same

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 12, 2019

Author Contributor

fixed

@@ -64,9 +48,9 @@ describe('Search', () => {
it('Search for Lisk ID using suggestions, signed in', () => {
cy.autologin(accounts.genesis.passphrase, networks.devnet.node);
cy.visit(urls.dashboard);
cy.get(ss.searchInput).click().type(`${accounts.delegate.address}`);
cy.get(ss.idResults).eq(0).click();

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 12, 2019

Contributor

You removed the essence of the test. Check the name of the test. What are you testing now?

cy.get(ss.searchIcon).click();
cy.get(ss.searchInput).type(`${accounts.delegate.username}`);
cy.get(ss.searchDelegatesRow).eq(0).click();
cy.get(ss.accountName).should('have.text', accounts.delegate.username);

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 12, 2019

Contributor

Correct here.

cy.get(ss.accountName).should('have.text', accounts['mainnet delegate'].username);
});

/**
* Search signed in mainnet
* @expect happens in mainnet
* This test should be fxied once the transaction details page
* change the behavior of how to present the data

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 12, 2019

Contributor

Ticket number? Bit more specific? I dont know what change are you talking about. Is the test broken now?

cy.get(ss.searchInput).click().type(`${accounts.delegate.address}{enter}`);
cy.get(ss.searchIcon).click();
cy.get(ss.searchInput).type(`${accounts.delegate.address}{enter}`);
cy.get(ss.searchAccountRow).eq(0).click();

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 12, 2019

Contributor

why both suggestion clicking and enter?

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 12, 2019

Author Contributor

fixed

cy.get(ss.emptyResultsMessage).should('have.text', 'No results');
cy.get(ss.searchIcon).click();
cy.get(ss.searchInput).type('321321');
cy.get(ss.searchMessage).eq(0).should('have.text', 'No results found.');

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 12, 2019

Contributor

This test was made for No Results Found screen. There is no way to get there now. It now duplicates preceding one. Test should be removed.

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 12, 2019

Author Contributor

fixed

@@ -4,34 +4,44 @@ import urls from '../../constants/urls';
import ss from '../../constants/selectors';
import regex from '../../../src/utils/regex';

const transferTxId = '12400920197376315040';

This comment has been minimized.

Copy link
@Efefefef

Efefefef Apr 12, 2019

Contributor

You somehow merged two conflicting versions of the file. Can you please roll back to the version from sprint branch?

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 12, 2019

Author Contributor

fixed

osvaldovega added some commits Apr 12, 2019

@osvaldovega

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Hi @Efefefef after review the search.spec.js file again I just realized that I made a big mistake with the classes and assertions as you mentioned, right now I just fixed all of them and confirm the asserting is making in a properly way, I remove unnecessary comments and additional data, please check it again and let me know if right now it is ok

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

👍 Very good

@Efefefef Efefefef added the ready label Apr 15, 2019

@osvaldovega osvaldovega merged commit f4045f0 into 1.16.0 Apr 15, 2019

4 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
security/snyk - package.json (LiskHQ) No new issues
Details

@osvaldovega osvaldovega deleted the 1774-implement-the-new-search branch Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.