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

feat: add feedback button #535

Merged
merged 2 commits into from
Apr 18, 2023
Merged

feat: add feedback button #535

merged 2 commits into from
Apr 18, 2023

Conversation

Plictox
Copy link
Collaborator

@Plictox Plictox commented Apr 11, 2023 •
edited
Loading

  • Kind: enhancement

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.

@erikmd erikmd added the kind: enhancement Enhancement to an existing user-facing feature. label Apr 11, 2023
@erikmd erikmd added this to the learn-ocaml 1.0.0 milestone Apr 11, 2023
@erikmd erikmd self-assigned this Apr 11, 2023
erikmd
erikmd requested changes Apr 11, 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 @Plictox !

I left a few comments for your PR

static/css/learnocaml_main.css Outdated Show resolved Hide resolved
static/css/learnocaml_main.css Outdated Show resolved Hide resolved
static/css/learnocaml_main.css Outdated Show resolved Hide resolved
static/index.html Outdated Show resolved Hide resolved
@erikmd
Copy link
Member

erikmd commented Apr 11, 2023

Suggestion (to tweak further w.r.t. spacing)

actually the spacing looks fine now.

Two leftover tasks:

  • Convert draw style from black to white
  • Add an id <img id="learnocaml-main-feedback" src="/icons/icon_feedback.svg"> to make it possible to add an attribute title="Send Feedback", internationalized, thanks to some js_of_ocaml code.

erikmd
erikmd approved these changes Apr 18, 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.

@Plictox, that looks very good to me, thanks!

Here is the outcome of your PR:

2023-04-18_19-12-04_Screenshot_a_img

→ note in passing that your CSS code induced a slight (unwanted) shift of the Learn OCaml text. This is only due to the margin-top: property, that is unnecessary.

Finally, note that the margin-left should rather be put outside the link <a href=_> rather than inside, so that the surrounding black space is not clickable.

I'll push a commit and rebase your pull request, so that we can get two final commits to be merged.

Thanks.

@erikmd
Copy link
Member

erikmd commented Apr 18, 2023

Now, let's wait for the CI ✅

@erikmd
feat(web-app): Add feedback button with internationalized tooltip
6b7b226 
Close ocaml-sf#525

Co-authored-by: Louis Tariot <louis.tariot@irit.fr>
Co-authored-by: Erik Martin-Dorel <erik.martin-dorel@irit.fr>
@erikmd erikmd merged commit 8878c9b into ocaml-sf:master Apr 18, 2023
@erikmd erikmd deleted the feedback branch April 18, 2023 19:14
@Plictox
Copy link
Collaborator Author

Plictox commented Apr 19, 2023

This feature could be improved by implementing a sidebar. It would be more complex but way more elegant ! cf the google feedback option
image

erikmd added a commit to pfitaxel/learn-ocaml that referenced this pull request May 1, 2023
@erikmd
fix(web-app): Fix process_html_file w.r.t. base_url
84bc393 
Don't prefix URLs starting with "https://" (or "http://").

Related: ocaml-sf#535 and static deployment
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
@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: enhancement Enhancement to an existing user-facing feature.
Projects
None yet
Milestone
learn-ocaml 0.15.0
Development

Successfully merging this pull request may close these issues.

Feature: Add feedback button in student UI ?
2 participants
@Plictox @erikmd