Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add hiive html attributes to ftc #20678
Add hiive html attributes to ftc #20678
Changes from 18 commits
0be2bfc
9b61693
1b9acd9
42ec4c2
04b6cca
6f57b11
8b14974
9bb9a87
7a39acd
0041fb2
805aec5
67da3e9
63659a6
6c3064f
d8c0dd9
cd3ab76
b3e5795
b8a9741
19dba25
447dab2
7a93fca
5616de8
e7c7032
6247a50
73de897
d881210
afeadba
54811af
317a3aa
042c33a
565f3c8
388d887
dbbaabb
7ef5da3
457253b
c9ee2d4
576dd23
a76b3ea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose by the difference
clicked_select_image
knows if it is a replace or not 🤔But it could be a separate event too, as you could argue the same for remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not remove it from
SITE_REPRESENTATION_FIELDS
?It seems to be a remnant from when we had a field for the
blogdescription
? Below there is the if/else to update that. And following what we send to the route in/packages/js/src/first-time-configuration/first-time-configuration-steps.js
'updateSiteRepresentation
- it is indeed not there.So unless I'm missing something, you could remove that instead of filtering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know (hence the comment there), this was just my super-cautious nature kicking in and the will to contain scope creep! 😅
Anyway, I removed the filter and the if/else related to the that field now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wpseo_ftc_post_update_site_representation
elsewhere!@internal
? Or create documentation PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_organization_social_profile_fields
uses but does not check if the filtered return value. Maybe add a safety(array)
there to ensure this keeps working?get_person_social_profile_fields
), but you haven't changed that code in your PR (set_person_social_profiles
method). Why?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first point has been addressed.
About the second point,
set_person_social_profiles
is dead code, I'm creating a tech debt issue to address that too. 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wpseo_ftc_post_update_social_profiles
elsewhere!@internal
? Or create documentation PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wpseo_ftc_post_update_enable_tracking
elsewhere!@internal
? Or create documentation PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're adding the return type hinting, why not also the argument?
(then I noticed the filters and missing type checks 😅 -- still, might want to make those safer instead)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.