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

Filter Buttons and Functions Rework #75

Merged
merged 7 commits into from
Dec 23, 2021

Conversation

undefined-landmark
Copy link
Contributor

@undefined-landmark undefined-landmark commented Dec 23, 2021

Hi,

A somewhat bigger pull request this time. Like I mentioned last time I wanted to do a rework of the filter functions, this led me to also rework the filter buttons.

Filter Buttons

splitk_filter_demo_720.mov

It somewhat comes down to personal preference but I found the radioButtons for the filter dropdown menu not that intuitive because of two main reasons:

  1. All filters are hidden in a dropdown menu and once you open it up you are overwhelmed by every filter option possible, even if you might just want to filter one thing.
  2. Almost every filter has the option all. Usually I assume that a non active filter automatically doesn't filter that variable. Therefore the all option was a bit redundant in my eyes.

For these two reason I opted for the pickerInput widget. With this widget all filter options are visible at a glance. When a filter is necessary this filter can be opened to show which options are available. A non active filter is equal to the All option as used previously. Because of the new buttons I had to also update the reset button, when updating this I tried to simplify this section by using a loop.

If you have a nicer solution for aligning all filter buttons instead of using <div>'s I'm happy to hear. It looks a bit more complicated than it has to be at the moment imo.

In addition the number of rows and number of keys filters now also accept a minimum number instead of only a maximum.

Filter Function

The filter functions related to the filter buttons currently have a one to one relationship, so 1 function for every button. This looked a bit complicated to me, as most filter options tend to do similar things. There are three main categories of filter buttons currently: those relating to one variable in the dataset (e.g. wireless), those relating to multiple variables (e.g. switch type: mxCompatible, chocV1, chocV2) and the range filters which are always active.

With these three categories in mind I made three functions. One for each categories. Hopefully the comments help out with the logic. The main idea was to make a logical of the dataset for every active filter. This involves checking if the filters are active, making the logical vector and combing the logical vectors of all active filters in the end. In the end this leads to less functions which was better to oversee imo. Adding a new filter should also require less code.

New Features

Whilst working on the code I found some new features that could be nice to have.

  • One thing would be to filter the DT in the Keyboards pane based on the filters of the compare pane. Adding a switch on the Keyboards pane to decide if you want to use the filters from the compare pane or show all.
  • Another simpler thing would be an open-source filter. Relatively easy to implement.

Code Formatting/Management

A couple questions:

  • Have you every thought about splitting app.R into ui.R, server.R and global.R? I found myself scrolling around a lot. Splitting the server and ui up might make it easier to navigate all code.
  • I'm pretty particular when it comes to code formatting. With my own code I usually use lintr and styler to make sure all my formatting is consistent. I'd like to use that here as well but I know this is a lot of personal preference. What do you think about linting the code?

That's it for this essay lol. If you have any comments/addition I'm happy to hear. Also the commit history is a bit of a mess because I tried squashing and merging things. When you merge this PR eventually squashing it would definitely be a good thing.

@jhelvy
Copy link
Owner

jhelvy commented Dec 23, 2021

Wow, great job on this! I appreciate the filter changes - I think they make for a better general experience and are easier to navigate than the prior pop-up window. The buttons themselves could maybe be improved. I was also thinking of using a different overall architecture, like shinydashboards or shiny.fluent. These seem to have a bit more structure and could provide a better overall layout for the menu items.

I'm not sure about the changes made to the filter function. The older structure was less efficient and a bit redundant, but it was very easy to follow the logic (one function per filter), and if you wanted to add a new filter it was pretty straightforward - just add another function and insert it. It required more lines of code, but they were very easy to read and understand. This new approach requires that you first understand the nature of the filter "types" (one-var, multi-var, or continuous) and then insert a new filter in the appropriate associated function. That being said, you only have to drop in the name as opposed to having to write a new function. There may also be some performance gains in how this is written. Given the work you put into it, I saw we keep it and move forward.

On the structure of the code, I usually use styler to style my code files. I would fine if you wanted to run that over the code to tidy things up. I also don't have a strong preference about breaking up the app into ui and server files. My only concern about that is whether or not the shiny::runGitHub('jhelvy/splitKbCompare') function would still work. I want to make sure that it can still recognize the app files.

I'm going to go ahead and merge this PR since it contains quite a lot of changes. If you want to implement these other changes (e.g., cleaning up the code, maybe tweaking the UI more, etc.), let's do that in another branch.

I'll also add your suggestions on other features to the todo.txt file.

@jhelvy jhelvy merged commit e9ec3a0 into jhelvy:master Dec 23, 2021
@undefined-landmark undefined-landmark deleted the filter_rework branch December 24, 2021 10:44
@undefined-landmark undefined-landmark mentioned this pull request Dec 24, 2021
19 tasks
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.

None yet

2 participants