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

fix: add search by nft and shorten search placehodler #626

Merged
merged 8 commits into from
Dec 21, 2023

Conversation

janmichek
Copy link
Collaborator

@janmichek janmichek commented Dec 15, 2023

Description

fixes #420
fixes #338
fixes #492

Demo

firefox_VxqfXMbZ0h.mp4

Checklist:

  • I have read and followed the Contributing Guide
  • fix nft hints
  • add search by nft
  • shorten placeholder

Copy link

@Liubov-crypto
Copy link

  1. The zoom is still here, but not as much as before. @janmichek , @michele-franchi , is this increase acceptable?
IMG_0875.MP4
  1. Searching is working differently on mobile and web, for example if I'm searching for 'next' on mobile I will get nothing.
IMG_0876.MP4

The same on web will get me a couple of names with 'next' in naming.
https://github.com/aeternity/aescan/assets/69896204/cd493745-4f74-43de-a444-d01b0ec0b771

@janmichek
Copy link
Collaborator Author

  1. The zoom is still here, but not as much as before. @janmichek , @michele-franchi , is this increase acceptable?

I tried to implement another fix. Please check it now

The same on web will get me a couple of names with 'next' in naming. https://github.com/aeternity/aescan/assets/69896204/cd493745-4f74-43de-a444-d01b0ec0b771

It's something I need to request from MDW
aeternity/ae_mdw#1651

@Liubov-crypto
Copy link

Liubov-crypto commented Dec 19, 2023

  1. The zoom is still here, but not as much as before. @janmichek , @michele-franchi , is this increase acceptable?

I tried to implement another fix. Please check it now

The same on web will get me a couple of names with 'next' in naming. https://github.com/aeternity/aescan/assets/69896204/cd493745-4f74-43de-a444-d01b0ec0b771

It's something I need to request from MDW aeternity/ae_mdw#1651

  1. The room on mobile has been fixed.

  2. I see that the search is sensitive to caps lock, for example, if I search for 'helo', it will not find NFT 'HELO'.

  3. I found an additional error during regression, I will register it separately.

  4. If you look at the activity of the selected account, the activity tabs, transactions, names, tokens will be unavailable:

2023-12-19.2.27.12.mov

The rest LGTM.

@janmichek
Copy link
Collaborator Author

I see that the search is sensitive to caps lock, for example, if I search for 'helo', it will not find NFT 'HELO'.

Yes that's what I requested to change on MDW side. Applies for all entities in search

I found an additional error during regression, I will register it separately.

Ok, thanks. Will fix separately

If you look at the activity of the selected account, the activity tabs, transactions, names, tokens will be unavailable:

I can take a look at this if it was a regression of this work. What is the account hash?

@Liubov-crypto
Copy link

@janmichek account hash ak_Cfr7h3XgjD6aScm3A96qMp2EF3p9Ao66gmVQvDEZAAsoCMGER

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at my comments.

Comment on lines 5 to 10
<th>
Symbol
<hint-tooltip>
{{ nftsHints.symbol }}
</hint-tooltip>
</th>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<th>
Symbol
<hint-tooltip>
{{ nftsHints.symbol }}
</hint-tooltip>
</th>

I would remove the "Symbol" column to keep it consistent with the NFTs page where we don't have it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@janmichek janmichek force-pushed the Search-field-placeholder-text-is-too-long branch from e571597 to 7bbbaaa Compare December 20, 2023 10:18
@janmichek
Copy link
Collaborator Author

@janmichek account hash ak_Cfr7h3XgjD6aScm3A96qMp2EF3p9Ao66gmVQvDEZAAsoCMGER

Fixed by rebase

Copy link

@Liubov-crypto Liubov-crypto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, LGTM

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job

@janmichek janmichek merged commit 8f94d04 into develop Dec 21, 2023
2 checks passed
@janmichek janmichek deleted the Search-field-placeholder-text-is-too-long branch December 21, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search by NFT Search field placeholder text is too long Input gets zoomed upon initialization of the page
3 participants