feat: Add some modifications to add direct link to filters#616
Open
CJs0800 wants to merge 6 commits intoocaml-sf:masterfrom
Open
feat: Add some modifications to add direct link to filters#616CJs0800 wants to merge 6 commits intoocaml-sf:masterfrom
CJs0800 wants to merge 6 commits intoocaml-sf:masterfrom
Conversation
erikmd
reviewed
Jul 7, 2025
src/app/learnocaml_index_main.ml
Outdated
Comment on lines
93
to
102
| let encode str = | ||
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp ",") ~subst:(fun _ -> "-c") ( | ||
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "&") ~subst:(fun _ -> "-a") ( | ||
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "=") ~subst:(fun _ -> "-e") ( | ||
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-") ~subst:(fun _ -> "--") str))) | ||
| let decode str = | ||
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "--") ~subst:(fun _ -> "-") ( | ||
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-e") ~subst:(fun _ -> "=") ( | ||
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-a") ~subst:(fun _ -> "&") ( | ||
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-c") ~subst:(fun _ -> ",") str))) |
Collaborator
There was a problem hiding this comment.
This code is correct, but it's possible to make it shorter/simpler, by applying these ideas:
- locally open the Re.Pcre module :
let open Re.Pcre inand replaceRe.Pcre.substitutewithsubstitute,Re.Pcre.regexpwithregexp; - reduce the need of parentheses by using the
@@operator → FYI,f @@ g @@ h etc = f (g (h etc))
Hence:
Suggested change
| let encode str = | |
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp ",") ~subst:(fun _ -> "-c") ( | |
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "&") ~subst:(fun _ -> "-a") ( | |
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "=") ~subst:(fun _ -> "-e") ( | |
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-") ~subst:(fun _ -> "--") str))) | |
| let decode str = | |
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "--") ~subst:(fun _ -> "-") ( | |
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-e") ~subst:(fun _ -> "=") ( | |
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-a") ~subst:(fun _ -> "&") ( | |
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-c") ~subst:(fun _ -> ",") str))) | |
| let encode str = | |
| let open Re.Pcre in | |
| substitute ~rex:(regexp ",") ~subst:(fun _ -> "-c") | |
| @@ substitute ~rex:(regexp "&") ~subst:(fun _ -> "-a") | |
| @@ substitute ~rex:(regexp "=") ~subst:(fun _ -> "-e") | |
| @@ substitute ~rex:(regexp "-") ~subst:(fun _ -> "--") | |
| @@ str |
And likewise for decode !
src/app/learnocaml_index_main.ml
Outdated
| | By_skill -> "skill" | ||
| | By_difficulty -> "difficulty" | ||
| in | ||
| update_fragment "sort" sort_value;set_exercise_sort sort;true); |
Collaborator
There was a problem hiding this comment.
Nitpick: add spaces here
Suggested change
| update_fragment "sort" sort_value;set_exercise_sort sort;true); | |
| update_fragment "sort" sort_value; set_exercise_sort sort; true); |
src/app/learnocaml_index_main.ml
Outdated
| let (exercise_sort_signal: exercise_ordering React.signal), set_exercise_sort = | ||
| React.S.create By_category | ||
|
|
||
| let (expand_state: string list React.signal), set_expand_state = |
Collaborator
There was a problem hiding this comment.
Just for uniformity, you could add the _signal suffix here as well:
Suggested change
| let (expand_state: string list React.signal), set_expand_state = | |
| let (expand_state_signal: string list React.signal), set_expand_state = |
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-a") ~subst:(fun _ -> "&") ( | ||
| Re.Pcre.substitute ~rex:(Re.Pcre.regexp "-c") ~subst:(fun _ -> ",") str))) | ||
|
|
||
| let rec update_expand ?value fragment = |
Collaborator
There was a problem hiding this comment.
It appears in your PR, there is no additional (* comments *) while IMO it'd be a good idea to:
-
Add a small global comment, for example before
encode, that briefly explains the feature added in the PR, its design, and the need to add the encode/decode mutual bijections… -
Add individual comments of one line or so just before each new function, to explain their usefulness (except if their semantics is trivial, e.g., no need to document the
joinlocal function):- if the new function at stake is public (it has a
val …in the.mli): document the function in the .mli only is OK - if the new function at stake is private (only in the
.ml): document the function in the .ml
- if the new function at stake is public (it has a
src/app/learnocaml_index_main.ml
Outdated
| let update_fragment key value = | ||
| let fragment = Js_utils.parse_fragment () in | ||
| let filtered_fragment = | ||
| if (key = "expand") then |
Collaborator
There was a problem hiding this comment.
Nitpick:
Suggested change
| if (key = "expand") then | |
| if key = "expand" then |
src/app/learnocaml_index_main.ml
Outdated
|
|
||
| let update_sort value fragment = | ||
| let filtered_fragment = List.remove_assoc "sort" (update_expand fragment) in | ||
| filtered_fragment@[("sort",value)] |
Collaborator
There was a problem hiding this comment.
Nitpick:
Suggested change
| filtered_fragment@[("sort",value)] | |
| filtered_fragment @ [("sort", value)] |
src/app/learnocaml_index_main.ml
Outdated
| ] ] | ||
| in | ||
| let update_expand_class id ids = | ||
| if (List.mem id ids) then |
Collaborator
There was a problem hiding this comment.
Nitpick:
Suggested change
| if (List.mem id ids) then | |
| if List.mem id ids then |
src/app/learnocaml_index_main.ml
Outdated
| | [] -> [] | ||
| | [_] -> | ||
| let expand_ids = React.S.value expand_state in | ||
| if (expand_ids = []) then |
Collaborator
There was a problem hiding this comment.
Nitpick:
Suggested change
| if (expand_ids = []) then | |
| if expand_ids = [] then |
src/app/learnocaml_index_main.ml
Outdated
| update_expand_class id expand_ids | ||
| | _ -> | ||
| let expand_ids = React.S.value expand_state in | ||
| if (expand_ids = []) then |
Collaborator
There was a problem hiding this comment.
Nitpick:
Suggested change
| if (expand_ids = []) then | |
| if expand_ids = [] then |
Comment on lines
+371
to
+372
| ignore (Manip.toggleClass title "collapsed"); | ||
| false); |
Collaborator
There was a problem hiding this comment.
This two lines were untouched by your PR except the indentation (spaces),
it might be better to restore the previous indentation so that the diff is not ambiguous (i.e., suggesting that the lines changed, while they didn't)
Add current files of my error situation
erikmd
added
the
kind: enhancement
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
feature
Description
This PR implements URL synchronization for the exercise index page:
This enables sharing direct links to specific filtered or expanded states.
Known issue:
Group expansions from URL fragments (#expand=...) are not applied correctly.