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

Post feedback to feedback api #303

Merged
merged 1 commit into from
Mar 6, 2025
Merged

Post feedback to feedback api #303

merged 1 commit into from
Mar 6, 2025
+101 −159

Conversation

dhee-tree
Copy link

@dhee-tree dhee-tree commented Feb 28, 2025 •
edited
Loading

What

  • Refactor to submit feedback form exclusively to dp-feedback-api via dp-api-router
  • Will be able to submit feedback to feedback api without feature flag

How to review

  • Run library
  • Run dp-frontend-homepage-controller
  • Check for feedback-api-url in elements on devtools and value is http://localhost:23200/v1/feedback

Who can review

!Me

@dhee-tree dhee-tree requested a review from a team as a code owner February 28, 2025 15:01
@dhee-tree dhee-tree force-pushed the feature/send-feedback-to-api branch 2 times, most recently from d4263b0 to 3b7f2ec Compare February 28, 2025 15:38
@dhee-tree dhee-tree changed the title Remove check for feedback api Post feedback to feedback api Mar 4, 2025
mr-nick17
mr-nick17 reviewed Mar 4, 2025
View reviewed changes
Copy link

@mr-nick17 mr-nick17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate you didn't write any of this code but let's try and tidy up what's left

  1. Replace all global var with either const or let
  2. Prefix all jQuery objects with a $

js/app/improve-this-page.js Outdated
feedbackURL = feedbackOrigin + feedbackURL;
}

var feedbackURL = document.querySelector("#feedback-api-url").value;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace the var with const and prefix jQuery objects with a $

Suggested change
var feedbackURL = document.querySelector("#feedback-api-url").value;
const $feedbackURL = document.querySelector("#feedback-api-url").value;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dhee-tree dhee-tree force-pushed the feature/send-feedback-to-api branch from 58a916a to e86623b Compare March 4, 2025 10:51
mr-nick17
mr-nick17 reviewed Mar 4, 2025
View reviewed changes
Copy link

@mr-nick17 mr-nick17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some unnecessary whitespace needs removing - double check the hooks on some as the space between selectors will be required

js/app/improve-this-page.js Outdated
}
e.preventDefault();

let id = $(this).attr("id");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let id = $(this).attr("id");
let $id = $(this).attr("id");

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

js/app/improve-this-page.js Outdated
hasErrors = true
}
e.preventDefault();
let $emailField = $(" #email-field ");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace?

Suggested change
let $emailField = $(" #email-field ");
let $emailField = $("#email-field");

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

js/app/improve-this-page.js Outdated
}
e.preventDefault();
let $emailField = $(" #email-field ");
let $nameField = $(" #name-field ");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

js/app/improve-this-page.js Outdated
e.preventDefault();
let $emailField = $(" #email-field ");
let $nameField = $(" #name-field ");
let $descriptionField = $(" #description-field ");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

js/app/improve-this-page.js Outdated
let $descriptionField = $(" #description-field ");
$descriptionField.removeClass("form-control__error");
$emailField.removeClass("form-control__error");
$(" .form-error ").each(function () {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more whitespace

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

js/app/improve-this-page.js Outdated
let hasErrors = false;

if ($descriptionField.val() === "") {
let descriptionError =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think this can be a const

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

js/app/improve-this-page.js Outdated
if ($descriptionField.val() === "") {
let descriptionError =
'<span class="form-error" role="alert">Write some feedback</span>';
if (!$(" #description-field-label .form-error").length) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more whitespace

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dhee-tree dhee-tree force-pushed the feature/send-feedback-to-api branch 2 times, most recently from b0a0e27 to 0fddf0e Compare March 4, 2025 15:14
mr-nick17
mr-nick17 reviewed Mar 5, 2025
View reviewed changes
Copy link

@mr-nick17 mr-nick17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last minor amends 👍

js/app/improve-this-page.js Outdated
feedbackURL = feedbackOrigin + feedbackURL;
}
const pageURL = window.location.href;
const $feedbackURL = document.querySelector("#feedback-api-url").value;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't actually a jQuery object (sorry!)

Suggested change
const $feedbackURL = document.querySelector("#feedback-api-url").value;
const feedbackURL = document.querySelector("#feedback-api-url").value;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

js/app/improve-this-page.js Outdated

if (!emailReg.test($email)) {
if (!$("#email-field-label .form-error").length) {
$("#email-field-label ").append(emailError);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace

Suggested change
$("#email-field-label ").append(emailError);
$("#email-field-label").append(emailError);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dhee-tree dhee-tree force-pushed the feature/send-feedback-to-api branch from 8221bdb to bc2c02c Compare March 6, 2025 09:18
@dhee-tree dhee-tree merged commit bc2c02c into main Mar 6, 2025
5 checks passed
@dhee-tree dhee-tree deleted the feature/send-feedback-to-api branch March 6, 2025 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mr-nick17 mr-nick17 mr-nick17 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dhee-tree @mr-nick17