-
Notifications
You must be signed in to change notification settings - Fork 34
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
1595/instance filtering #1610
Closed
chrissolanilla
wants to merge
10
commits into
ucfopen:dev/10.3.0
from
chrissolanilla:1595/Instance-Filtering
Closed
1595/instance filtering #1610
chrissolanilla
wants to merge
10
commits into
ucfopen:dev/10.3.0
from
chrissolanilla:1595/Instance-Filtering
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…1595/Instance-Filtering
clpetersonucf
requested changes
Sep 30, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few high-level comments:
- You have commits from the unrelated 1600/save draft button #1602 PR on this branch/PR
- The loading throbber and associated visual changes when widget instances are being requested are broken. These elements would overlay the search bar until instance loading is complete.
- The filter buttons don't really work in their current position below the search bar. I would rather the filter options remain hidden within an element that expands within the sidebar when toggled. I can work with you to figure out how that might look.
- Draft and Published filter options are good.
attempts
has nothing to do with whether an instance possesses scores; it's the setting associated with an instance's attempt limit. It's set via the settings dialog in My Widgets (under Collaborate/Copy/Delete). That said, filtering by instances that possess an attempt limit is a good add. - Similarly, filtering by instances with open/close dates would be ideal.
I appreciate the effort you made in getting score information for each instance, but there are some significant issues with your approach:
- Memoizing the instance score information is good, but please conform to our use of react query to facilitate API requests in the future
- I am very much not a fan of requesting
apiScoreSummaryGet
for each instance that's received. Users can potentially have hundreds of instances, which results in hundreds of requests to the server happening all at once. Instead, I think we should consider appending some minimal score information to widget instance objects themselves on the server as part of the response towidget_paginate_user_instances_get
. The least impactful version is probably a score count (viaCOUNT
fromlog_play
associated with aninst_id
) but I'd also prefer that value isn't even included unless the API request specifically asks for it (via another parameter in thewidget_paginate_instances_get
call). This would mean potential changes to theapi_v1
,widget_instance_manager
, andwidget_instance
classes.
Closing, replaced with #1614 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this pull request I have added 3 buttons underneath the My Widget Page in Materia for more advanced filtering.
Changes
I ended up changing the styles of the sidebar to be more flexible(no pun intended) so it makes more sense and so that it can comply with my button additions.
For the actual filters I made them check boxes with a parent of a clickable div. They change color when clicked or when you press the keyboard when they are focused. When they are focused I gave them a nice blue highlight to the parent div similar to how I did it in connections.
The three filters I added are
Drafts:On/Off
,Published:On/Off
andHas Score:On/Off
These filters will filter based on the properties of the instance object for
instance
with properties ofdraft
for both buttonsDrafts: On/Off
andPublished: On/Off
. For the filterHas Score:On/Off
, I made an api call to get the scores instead of looking at the properties of the instance since theattempts
property seems to always be -1.