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.

Sign up for GitHub

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

Jump to bottom

Hide tokens in partition-view.html #540

Merged
merged 27 commits into from May 1, 2023
Merged

Hide tokens in partition-view.html #540

merged 27 commits into from May 1, 2023

Conversation

Plictox
Copy link
Collaborator

@Plictox Plictox commented Apr 20, 2023

  • Kind: feature

Description

Checklist

Note to maintainers

  • Read this wiki page.
  • Make sure the PR has a milestone.
  • Assign yourself before merging.
  • Either do a regular merge:
    • for PRs containing several commits following conventional-commits,
    • or for PRs containing 1 commit shared with a later PR (to preserve the SHA1)
  • Or do a squash-merge:
    • for PRs containing only 1 commit (not shared with a later PR),
    • or for PRs containing several commits that need not be kept in the history;
    • → Update the commit message header with a conventional-commit type,
    • → Add a footer Close #… if a related issue exists.

@Plictox Plictox assigned Plictox and unassigned Plictox Apr 20, 2023
@Plictox Plictox requested a review from erikmd April 20, 2023 10:39
@erikmd erikmd added the kind: feature New user-facing feature. label Apr 20, 2023
erikmd
erikmd reviewed Apr 23, 2023
View reviewed changes
static/partition-view.html Outdated Show resolved Hide resolved
erikmd
erikmd reviewed Apr 23, 2023
View reviewed changes
static/partition-view.html Outdated Show resolved Hide resolved
erikmd
erikmd reviewed Apr 23, 2023
View reviewed changes
Copy link
Member

@erikmd erikmd left a 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.

@erikmd erikmd mentioned this pull request Apr 26, 2023
Closed
erikmd
erikmd requested changes Apr 26, 2023
View reviewed changes
Copy link
Member

@erikmd erikmd left a 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!

src/app/learnocaml_partition_view.ml Outdated Show resolved Hide resolved
src/app/learnocaml_partition_view.ml Outdated Show resolved Hide resolved
static/partition-view.html Outdated Show resolved Hide resolved
static/partition-view.html Outdated Show resolved Hide resolved
src/state/learnocaml_data.ml Outdated Show resolved Hide resolved
src/app/learnocaml_partition_view.ml Outdated Show resolved Hide resolved
src/app/learnocaml_partition_view.ml Outdated Show resolved Hide resolved
static/css/learnocaml_common.css Outdated Show resolved Hide resolved
src/app/learnocaml_partition_view.ml Outdated Show resolved Hide resolved
src/app/learnocaml_partition_view.ml Outdated Show resolved Hide resolved
erikmd
erikmd reviewed Apr 26, 2023
View reviewed changes
src/app/learnocaml_partition_view.ml Outdated Show resolved Hide resolved
src/app/learnocaml_partition_view.ml Outdated Show resolved Hide resolved
Plictox and others added 4 commits April 27, 2023 09:54
@Plictox
feat: changes from review
32ea5e1
@Plictox
feat: useless code deleted
f4fdd8f
@Plictox @erikmd
feat: changing variable names
c1f2f2f 
Co-authored-by: Erik Martin-Dorel <erik@martin-dorel.org>
@Plictox @erikmd
feat: improved list_of_students_details
1d3abf4 
Co-authored-by: Erik Martin-Dorel <erik@martin-dorel.org>
@Plictox
Copy link
Collaborator Author

Plictox commented Apr 27, 2023

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.
The rest should be fine.

@erikmd
Copy link
Member

erikmd commented Apr 27, 2023

@Plictox OK good! I'll take a look ASAP.

Meanwhile, feel free to start analyzing #527 :)

@erikmd erikmd self-assigned this Apr 27, 2023
@erikmd erikmd added this to the learn-ocaml 1.0.0 milestone Apr 27, 2023
@erikmd erikmd changed the title Hide tokens Hide tokens in partition-view.html Apr 27, 2023
erikmd
erikmd approved these changes Apr 30, 2023
View reviewed changes
Copy link
Member

@erikmd erikmd left a 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…

@erikmd
Copy link
Member

erikmd commented Apr 30, 2023

As a heads-up, here is a screenshot of the resulting view:

2023-04-30_23-54-31_Screenshot_partition-view

@erikmd erikmd merged commit 58b3644 into ocaml-sf:master May 1, 2023
14 checks passed
@erikmd erikmd deleted the hide_tokens branch May 1, 2023 09:37
@github-actions github-actions bot mentioned this pull request May 1, 2023
Merged
@erikmd erikmd mentioned this pull request May 1, 2023
Merged
5 tasks
erikmd added a commit that referenced this pull request May 1, 2023
@erikmd
Merge pull request #547 from pfitaxel/dont-prefix-absolute-urls
1584a08 
* Don't prefix URLs starting with `https?://` at `learn-ocaml build` time
Related: #535 and static deployment

* Move CSS code as a small cleanup
Related: #540
@Plictox
Copy link
Collaborator Author

Plictox commented May 2, 2023

dear erik, it looks great ! And thanks for fixing the issues!

erikmd added a commit to pfitaxel/learn-ocaml that referenced this pull request Jun 15, 2023
@erikmd
Merge branch 'master' into oauth-moodle-dev
9694dd9 
* 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
@erikmd erikmd mentioned this pull request Aug 23, 2023
Merged
@erikmd erikmd mentioned this pull request Nov 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikmd erikmd erikmd approved these changes

Assignees

@erikmd erikmd

Labels
kind: feature New user-facing feature.
Projects
None yet
Milestone
learn-ocaml 0.15.0
Development

Successfully merging this pull request may close these issues.

Feature: Add a selector to hide tokens from partition-view pages
2 participants
@Plictox @erikmd