Skip to content

Conversation

@JesusValeraDev
Copy link
Member

@JesusValeraDev JesusValeraDev commented Jan 16, 2022

Reference to the issue #8

📚 Description

The idea of this PR is to create an offline search that contains all Phel API symbols with the signature and the description.
In order to get that, we will implement elasticlunr() tool, for that we need to build manually the index.

To create this file, you have to build manually the API page (as the README.md file mentions).

composer build

TIP: As the previous command is executed automatically on every release, we don´t need to trigger anything in the future to dump the search API database 😄

Also, the content/documentation/api.md was added to .gitignore because this file is generated on the fly.

💡 Demo

image

image

@JesusValeraDev JesusValeraDev added the enhancement New feature or request label Jan 16, 2022
@JesusValeraDev JesusValeraDev self-assigned this Jan 16, 2022
@Chemaclass
Copy link
Member

If the static/api_search.js is autogenerated, shouldn't it be ignored by git?

@Chemaclass Chemaclass marked this pull request as draft January 17, 2022 14:29
@Chemaclass Chemaclass self-assigned this Jan 17, 2022
@jenshaase
Copy link
Member

jenshaase commented Jan 17, 2022

I took a quick look. I think this a good improvement to the site. However, I have some suggestion:

  • The search bar in the header is not fully align with the text of the navigation
  • Have you though about to move the search box in the middle of the navigation bar?
  • Remove the grey separation line after the last found entry
  • Make the complete area of the list item clickable not just the function name.
  • Mobile variant is not optimal (very small container).
  • Why is the zola search feature enabled (build_search_index) if we only search in our own search index?

@JesusValeraDev
Copy link
Member Author

JesusValeraDev commented Jan 17, 2022

I took a quick look. I think this a good improvement to the site. However, I have some suggestion:

  • Remove the grey separation line after the last found entry
  • Make the complete area of the list item clickable not just the function name.
  • Why is the zola search feature enabled (build_search_index) if we only search in our own search index?
  • Have you though about to move the search box in the middle of the navigation bar?

Done.

image

image

  • The search bar in the header is not fully align with the text of the navigation

I think it's already align horizontal & vertical, is that what you mean?

  • Mobile variant is not optimal (very small container).

It's a bit bigger now, what do you think @jenshaase?

image

@Chemaclass Chemaclass marked this pull request as ready for review January 18, 2022 09:22
@JesusValeraDev
Copy link
Member Author

JesusValeraDev commented Jan 18, 2022

I create a new issue in the elasticlunr() library about allowing special characters: weixsong/elasticlunr.js#136

Edit: Solved!

@Chemaclass Chemaclass force-pushed the feature/search-field branch from 04f9d4e to 7b33c47 Compare January 18, 2022 20:57
Copy link
Member

@Chemaclass Chemaclass left a comment

Choose a reason for hiding this comment

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

Maybe @jenshaase has some style adjustments in mind, but I think this PR is an awesome achievement.
A searcher in any "documentation website" is a must 👌🏻

@jenshaase
Copy link
Member

Can we remove the markdown formats (see screenshot)?

screenshot-2022-01-19_1600x900

I will try to optimize the CSS a little bit tomorrow.

@JesusValeraDev
Copy link
Member Author

JesusValeraDev commented Jan 19, 2022

Can we remove the markdown formats (see screenshot)?

@jenshaase it should be fixed here.
git pull, then run composer build to generate a new api_search.js file and try again.

image

@jenshaase
Copy link
Member

I just improved the desktop version of the top navigation. Will work on the mobile version tomorrow. What do you think?

screenshot-2022-01-20_1600x900

I also found a few thinks that could be improved:

  • A notice should be given if now result if found. Currently the drop down is simply not showing up.
  • If I search something, then click somewhere else on the screen and then click back in the search field, the drop down is not showing up. I first have to type a letter before it will show the drop down. Alternative solution would be to clear the search field whenever I leave the field.

@JesusValeraDev
Copy link
Member Author

JesusValeraDev commented Jan 21, 2022

I just improved the desktop version of the top navigation. Will work on the mobile version tomorrow. What do you think?

I like the new design 👌🏼

I also found a few thinks that could be improved:

  • A notice should be given if now result if found. Currently the drop down is simply not showing up.
  • If I search something, then click somewhere else on the screen and then click back in the search field, the drop down is not showing up. I first have to type a letter before it will show the drop down. Alternative solution would be to clear the search field whenever I leave the field.

I updated the JS implementing both requirements, tell me what you think :)

image

@jenshaase
Copy link
Member

I tried to implement the a mobile version of the search. If I want to put it in the mobile menu, I have to use another search input field with another search results div inside <div class="site-header__mobile-menu">. However, with the current JS this is not working without bigger changes.

Therefore, I would propose that we first finish the PR with a search for the desktop version only and then work on the mobile version in another PR. What do you think?

@Chemaclass
Copy link
Member

Chemaclass commented Jan 21, 2022

This PR is getting too big and we should close it to get its value, and work on continuous improvements in a follow up PR.
Let's keep the search field for the mobile in a follow up PR, and display it only for desktop.

@JesusValeraDev
Copy link
Member Author

JesusValeraDev commented Jan 21, 2022

@jenshaase yes! Completely agree.
I would suggest to merge this PR and we can work on the search for mobile version in the future.

You can merge it 👍🏼

@jenshaase jenshaase merged commit b76e48b into master Jan 21, 2022
@jenshaase
Copy link
Member

Merged!

@Chemaclass Chemaclass deleted the feature/search-field branch January 21, 2022 21:28
@JesusValeraDev
Copy link
Member Author

I tried to implement the a mobile version of the search. If I want to put it in the mobile menu, I have to use another search input field with another search results div inside <div class="site-header__mobile-menu">. However, with the current JS this is not working without bigger changes.

Therefore, I would propose that we first finish the PR with a search for the desktop version only and then work on the mobile version in another PR. What do you think?

Is anything new from your side @jenshaase?
Maybe you could create a scratch and we can together discuss and work on it.

@JesusValeraDev
Copy link
Member Author

CURRENT STATUS:
If we comment L75 on _header.scss -> display: none;, we have the following:

Screen.Recording.2022-10-28.at.19.55.51.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants