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

feat: use explorer page as search for mobile only #1301

Merged
merged 11 commits into from
Feb 3, 2023

Conversation

edimitchel
Copy link
Collaborator

@edimitchel edimitchel commented Jan 18, 2023

Description

Merge usage of search and explore in one only for mobile or medium screen (could have both search and explore icon active for medium screen).

Resolves #1258
Resolves #894
Resolves #1560

explore-search-2


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Translations update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide related snapshots or videos.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).

@stackblitz
Copy link

stackblitz bot commented Jan 18, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Jan 18, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit bfbf72a
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/63daf5e342c5bd00086f598d

@netlify
Copy link

netlify bot commented Jan 18, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit bfbf72a
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/63daf5e3ef4b9000090d088f
😎 Deploy Preview https://deploy-preview-1301--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ZMYaro
Copy link

ZMYaro commented Jan 18, 2023

As I said on #1274, I think it would make more sense to follow Twitter's example and have the Search icon replace the Explore icon in medium size windows instead of being a redundant link to the same page.

@edimitchel
Copy link
Collaborator Author

As I said on #1274, I think it would make more sense to follow Twitter's example and have the Search icon replace the Explore icon in medium size windows instead of being a redundant link to the same page.

Applied.

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

Looks nice!

Copy link
Member

@boehs boehs left a comment

Choose a reason for hiding this comment

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

I think that "Explore" should be hidden in the mobile submenu, because they are the same page. Otherwise, LGTM

@patak-dev
Copy link
Member

Let's merge this one so we avoid the need to resolve conflicts and iterate. I think we'll need to review how this will look if we show search results similar to the timeline more like a dialog for mobile later.

@patak-dev patak-dev merged commit 4b1b187 into main Feb 3, 2023
@patak-dev patak-dev deleted the feat/search-on-mobile branch February 3, 2023 10:40
@TodePond
Copy link
Contributor

TodePond commented Feb 3, 2023

Hey just to chime in on this - I feel like there should be a setting to toggle this as desired by the user. I really like having 'explore' hidden away, as to not get in the way when just trying to do a simple search. On twitter, I understood why they might want to shove trending posts in my face, but it has been very refreshing to not have that on mastodon.

I think many other people appreciate that about mastodon VS twitter as well :)
Explained a bit further back here: #597

Anyone else relate?

@patak-dev
Copy link
Member

I agree here, I was also hesitant about making this change but it is good that we tried to get some more data. There were a few messages from folks confused by it on Mastodon. I think we should revert and have to explore and search separately again and let people explicitly go to explore when they want to discover content (I don't use explore at all for example, so I also prefer not to see it).

About having an option to merge them, I think it is better to avoid it for now as we already have too many options, and it isn't clear to me that a lot of people are asking for this.

This PR introduces others changes too that are great (sticky user in the sidebar, etc), @edimitchel let me know if you'd like to send a revert only for the merging part, if not I'll get to it later today or tomorrow

@edimitchel
Copy link
Collaborator Author

I can do a revert but I would like to suggest another thing: on mobile, when search is clicked only show the search input and focus to it.
When focus is lost or scroll is made more than 100px, the search input would be hidden.
Also, I think the search result should not be a popper but should take place on the body.
I'll revert the merge now then work on that feature if it sounds good for you.

@patak-dev
Copy link
Member

Yes, I think we can revert then keep exploring how to improve things.

@TodePond
Copy link
Contributor

TodePond commented Feb 4, 2023

That sounds like a good plan!

Either way, thanks for putting in time to explore these options @edimitchel, I appreciate it! It's useful to see features like this actually implemented, to get a better understanding of them 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants