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

fix: Content: SEO Meta Description Displays that there are excess characters when there are none #2534

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

glespinosa
Copy link
Contributor

@glespinosa glespinosa commented Feb 5, 2024

This will also solve problem in Meta Title, Meta Keywords, Navigation Link Text not only in the Meta Descriptions because they share same issues regarding excess characters

screencast-8-aaeffee09b-7w6v22.manager.dev.zesty.io_8080-2024.02.06-09_19_45.webm

Closes #2501 #2502

@glespinosa glespinosa marked this pull request as draft February 5, 2024 22:38
@glespinosa glespinosa changed the title seo meta excess errors fix: Content: SEO Meta Description Displays that there are excess characters when there are none Feb 6, 2024
@glespinosa glespinosa self-assigned this Feb 6, 2024
@glespinosa glespinosa added the bug Something isn't working label Feb 6, 2024
@glespinosa glespinosa requested a review from agalin920 February 6, 2024 01:23
@glespinosa glespinosa marked this pull request as ready for review February 6, 2024 01:23
@glespinosa glespinosa requested a review from finnar-bin February 7, 2024 02:50
Copy link
Contributor

@finnar-bin finnar-bin left a comment

Choose a reason for hiding this comment

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

@glespinosa did you check with @zcolah if the expected behavior here is to automatically trim the user input instead of retaining the excess characters and letting the user fix it themselves before it gets saved?

@zcolah
Copy link

zcolah commented Feb 7, 2024

@glespinosa did you check with @zcolah if the expected behavior here is to automatically trim the user input instead of retaining the excess characters and letting the user fix it themselves before it gets saved?

@theofficialnar currently this is the approach we are taking as it is the behavior that was already being followed for the other fields on the SEO Tab. We definitely want an error blurb like experience for SEO in the near future though.

Copy link
Contributor

@agalin920 agalin920 left a comment

Choose a reason for hiding this comment

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

The amount of code duplication and the heavy reliance on useEffect makes me think this is not a great solution.

@@ -40,6 +41,10 @@ export default connect()(function MetaDescription({
}

setError(message);

if (isSaving) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You are accessing isSaving variable but it's not a dependency of the useEffect?

Copy link
Contributor Author

@glespinosa glespinosa Feb 12, 2024

Choose a reason for hiding this comment

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

@agalin920 yes for now it's relying on the useEffect since everytime the user is saving that's the only time I can rerun the onChange function to check the characters condition on excess. I'm adding it now as dependency on useEffect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agalin920 I removed now the useEffect on every component and let the useEffect inside of ItemSettings do the work of removing the errors while on there are on saving... Then i slice all the props base on the number of allowed character on the properties in the /shell/store/content.js

item.web.metaLinkText = item.web.metaLinkText.slice(0, 150);
}
if (item.web.metaKeywords) {
item.web.metaKeywords = item.web.metaKeywords.slice(0, 250);
Copy link
Contributor

@finnar-bin finnar-bin Feb 12, 2024

Choose a reason for hiding this comment

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

Why is it 250 here? The ui's character counter says it's 255. Probably worth checking in with @markelmad what the expected char limit here is.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done updating changes

Copy link
Contributor

Choose a reason for hiding this comment

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

@glespinosa which character count did we settle on and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shrunyan I'm basing on the UI
image

@glespinosa glespinosa requested a review from agalin920 February 12, 2024 23:50
@zcolah zcolah added the vqa VQA is complete and approved label Feb 15, 2024
@shrunyan shrunyan merged commit 0df5e4f into master Feb 21, 2024
1 check failed
@shrunyan shrunyan deleted the fix/excess-char-seo-desc branch February 21, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vqa VQA is complete and approved
Projects
None yet
5 participants