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.

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

Feature / update comment error message #161

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

sbrody
Copy link
Contributor

@sbrody sbrody commented Feb 21, 2024

Changes markup for comment error messages in line with update to 4.0. Can be tested once this PR has been approved.

@RobjS
Copy link
Contributor

RobjS commented Feb 21, 2024

@sbrody Do you know how I can trigger this markup to appear in the frontend?

@sbrody
Copy link
Contributor Author

sbrody commented Feb 21, 2024

@sbrody Do you know how I can trigger this markup to appear in the frontend?

Sorry - yes should have added some testing notes. Go to a single blog post and attempt to comment with an empty message and you will see the error markup.

@sbrody sbrody force-pushed the feature/update-error-message-html branch from 71749d7 to 33a65ca Compare February 21, 2024 14:55
@sbrody
Copy link
Contributor Author

sbrody commented Feb 21, 2024

@RobjS I've rebased in the changes setting jQuery as a dependency. On doing that I discovered a couple more errors in the JavaScript caused by improper use of $ - which I have also fixed here as they are related to the commenting issue.

@RobjS
Copy link
Contributor

RobjS commented Feb 21, 2024

@sbrody I'm a bit unsure what's happening here! Before you pushed the new changes, I couldn't get the errors to trigger, though I'm not sure why. But with the new changes you've pushed, I can get them to trigger even without the changes from $ to jQuery.

i.e. if I do git checkout 6930d to get just your first commit, then run yarn run grunt to re-compile the JS, the errors still work. So I think the $/jQuery change might be a red herring?

@sbrody
Copy link
Contributor Author

sbrody commented Feb 21, 2024

@sbrody I'm a bit unsure what's happening here! Before you pushed the new changes, I couldn't get the errors to trigger, though I'm not sure why. But with the new changes you've pushed, I can get them to trigger even without the changes from $ to jQuery.

i.e. if I do git checkout 6930d to get just your first commit, then run yarn run grunt to re-compile the JS, the errors still work. So I think the $/jQuery change might be a red herring?

@RobjS yes - the errors are displaying but are you not getting the console error, which prevents the error summary box above from appearing? (Although not related to the update, I thought it was worth fixing at the same time - shall I separate it out into another PR?)

I should add - you are now seeing the error message because of the separate PR setting jquery as a dependency, which I rebased in to this PR as it's now been approved. I thought that would help with testing but it perhaps just created more confusion!

Copy link
Contributor

@RobjS RobjS left a comment

Choose a reason for hiding this comment

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

Ah, that all makes sense now! LGTM 👍

@sbrody sbrody merged commit 7091d3e into master Feb 21, 2024
3 checks passed
@sbrody sbrody deleted the feature/update-error-message-html branch February 21, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants