Move sorting options out of the navbar

[Dosbodoke]

fix bastislack/highline-freestyle#231

Link to issue

Correct me if this way of creating a PR is not correct, never done PR to open-source before.

What is the purpose of this PR:

Fix the Navbar responsiveness by approaching the proposed solution.

Move the sorting options out of the navbar. Therefore the navbar becomes so short that the issue will not occur under any reasonable circumstances anymore. The sorting options could be moved next to or underneath the search bar (maybe depending on the screen size), as a drop down menu.

What was done to achieve

• Created a SearchBar component that has both text-based filtering and sort filtering
• Since the component SearchBar was already created and the sort filtering would need to be implemented in the Combos page, I enhanced it with text-based filtering [(not fully implemented)](#Know limitation)
• Removed all logic of sorting from both LeftNav and TopNav
• Removed sortOpt state from App.js, it should now reduce unnecessarily re-renders

How to test if it works?

Open the Tricks Page and see if it looks like this:

• Use the text search bar and check if behaves as previously.
• Use the new filter dropdown and check if behaves as previously.

Now use Ctrl + Shift + M to simulate a mobile view and check if the responsiveness is good enough.

Know limitation

I couldn't make the text-based filtering work on ComboList.js so I added a comment on line 30: // TODO: Finish filtering functionality

[Dosbodoke]

I agree with Julian's comment, we need to resolve this Task before merging

Lastly on a general note, I think it would be good to find out how to get the text-based filtering to work on the ComboList before merging. Otherwise we will have a "broken" feature on the develop branch. Maybe someone else has an idea how to do this.

• [x] Get the text-based filtering to work on the ComboList.

And I think that a linter like Prettier would be great to prevent the need for manual code style revision like

Add semicolon

Apparently, this is being implemented on this forked repository

[JulianDietzel]

I think I figured out how to get the search to work. In ComboList.js where your TODO comment is try using the following three lines (only the first two are changed, the last one is unaltered):

const searchOptions = { keys: ["name", "establishedBy"] };
const fuse = new Fuse(combos, searchOptions);
const searchResults = searchPattern ? fuse.search(searchPattern).map(i => i.item) : combos;

This should make the search look for the names of the combos and the names of the people who established them.

[Dosbodoke]

Seems good. will commit

[JulianDietzel]

Right after commenting I corrected myself. See the following line:

const searchOptions = { keys: ["name", "establishedBy"] };

You're just too fast for me :D

[Dosbodoke]

I trusted you bro :disappointed:, will fix that haha.

[Dosbodoke]

Did a force push in order to not mess up a lot with the commits

[JulianDietzel]

Also feel free to mark my comments as resolved when you implemented them. (I tend to reply with "Implemented" or something along the lines before resolving the comment so it is clear what the result was. I don't know if this is the best practice approach, but it seems somewhat sensible)

[Dosbodoke]

I'm used to work with BitBucket and there we create tasks that need to be marked as resolved in order to allow merge, I think that there is something similar here.

GitHub - Task list completed PR check

Don't know if it has a native approach for this.

[JulianDietzel]

Resolving the comments is the standard GitHub way to handle this. There are also changes that need to be done before the merge can happen, which can be seen in Pull Request 198 at the moment. My comment on not knowing what the best practice way is, was related to whether to write a brief one word response to the request before closing it or not (but in all honesty, who cares?).

[Dosbodoke]

What is missing for the merge?

[JulianDietzel]

Time. This is not a professional project, so it can take a moment for someone to find the time to check the PR. This PR also affects quite a number of files so checking all of them can't simply be done in a few minutes.

[bastislack]

Looks good! Thank you so much for your work :)