-
Notifications
You must be signed in to change notification settings - Fork 67
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
Hide tokens in partition-view.html #540
Conversation
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.
Dear @Plictox, that looks like a good start.
Here is a review of the current version of your PR.
Co-authored-by: Erik Martin-Dorel <[email protected]>
…de_tokens * 'hide_tokens' of github.com:pfitaxel/learn-ocaml: feat: hide PII feature
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.
thanks for your commits → here are new review comments!
Co-authored-by: Erik Martin-Dorel <[email protected]>
Co-authored-by: Erik Martin-Dorel <[email protected]>
Co-authored-by: Erik Martin-Dorel <[email protected]>
I finished implementing correctly the feature. There is however a last small issue, the option selected by default is not the one I want (nickname-id). It keeps displaying "Hide PII" in the select button after I load the page. |
Now: 1. "List" 2. "Details" 3. "Answer"
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.
Dear @Plictox, good news! I fixed all the issues that we were talking about, then improved the CSS and the .ml code a bit further (already applied the suggestions I spotted during my review).
To sum up, the PR is ready, but let's wait that the CI does its checking, and let's merge on tomorrow…
dear erik, it looks great ! And thanks for fixing the issues! |
* master: (146 commits) chore(docker): Fix org.opencontainers.image.source label ci(docker): Replace `LABEL` Dockerfile commands with `labels:` (GHA) (ocaml-sf#551) fix(i18n): fix escaping issue in i18n fix(ui): update fr translation feat(ui): add some inline documentation to the teacher tab feat(ui): teacher tab: highlight the "apply" button on unsaved changes fix(ui): show different status for open and closed assigned exercises feat(ui): allow partial CSV export feat(ui): allow name input on teacher token creation feat(ui): better string input dialog chore: Add 2 checkboxes in PULL_REQUEST_TEMPLATE.md fix(web-app): Fix `process_html_file` w.r.t. `base_url` refactor(partition-view): Move adhoc CSS code to learnocaml_partition_view.css ci(docker): Fix build-args syntax (docker/build-push-action@v4) ci(docker): Fix GHA input name: s/build_args/build-args/ ci(docker): Use docker/build-push-action@v4 (ocaml-sf#544) ci(macos): Run the `macOS` workflow as well in the weekly CI build feat(partition-view): Add a selector to show (tokens, nicks, or anon IDs) (ocaml-sf#540) feat(web-app): Add feedback button with internationalized tooltip feat(js_utils): Add HTMLElement.title support ... Conflicts: .ci-macosx.sh .github/workflows/static-builds.yml ci/docker-emacs-learn-ocaml-client/.emacs dune-project learn-ocaml-client.opam learn-ocaml.opam learn-ocaml.opam.locked scripts/static-build.sh src/app/learnocaml_common.ml src/app/learnocaml_common.mli src/app/learnocaml_description_main.ml src/app/learnocaml_index_main.ml src/app/learnocaml_teacher_tab.ml src/app/server_caller.ml src/main/dune src/main/learnocaml_client.ml src/main/learnocaml_server_main.ml src/main/linking_flags.sh src/server/learnocaml_server.ml src/state/learnocaml_api.ml src/state/learnocaml_api.mli src/utils/dune static/css/learnocaml_main.css static/index.html translations/fr.po
Description
Checklist
Note to maintainers
Close #…
if a related issue exists.