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

Ensure no errors on CPTs without Yoast SEO metabox #103

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Ensure no errors on CPTs without Yoast SEO metabox #103

wants to merge 5 commits into from

Conversation

kraftner
Copy link
Contributor

fixes #88

andizer
andizer reviewed Oct 20, 2017
View reviewed changes
Copy link
Contributor

@andizer andizer left a comment

Choose a reason for hiding this comment

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

I've done a codereview and have some small comment.

inc/class-ac-yoast-acf-content-analysis.php Outdated
@@ -223,4 +227,20 @@ protected function get_blacklist_type() {
protected function get_blacklist_name() {
return new Yoast_ACF_Analysis_String_Store();
}

/**
* Adds a CPT for tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this line more clear. I know where CPT stands for, but somebody who's new to the codebase probably won't know.

inc/class-ac-yoast-acf-content-analysis.php Outdated


/**
* @return bool
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have some documentation here.

tests/js/system/data/acf4.php
@@ -100,4 +100,42 @@
),
'menu_order' => 0,
));

register_field_group(array (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a space before array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is coming out of the ACF Export function like this I'd rather leave it like this as it makes it easy to just C&P stuff on changes and having clean diffs. What do you think about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

That 's okay for me 👍

tests/js/system/data/acf5.php
@@ -336,4 +336,50 @@
'description' => '',
));

acf_add_local_field_group(array (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a space before array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above.

@kraftner kraftner mentioned this pull request Oct 20, 2017
Merged
@andizer
Copy link
Contributor

andizer commented Oct 23, 2017

CR Done 👍

@moorscode
Copy link
Contributor

I'm having an issue with the testing code being inside the plugin, but I understand that the total approach needs to be revised in order to change this.

As we don't want to ship the tests or any non-build files in the actual plugin, we would be referencing code that does not exist.

@kraftner
Copy link
Contributor Author

I absolutely agree with that. This was kind of ok while it was just one simple thing. But as this grows we'll need to get this out at some point.

@moorscode
Copy link
Contributor

See #111

@kraftner
Copy link
Contributor Author

I think the question here and now is if we get this out and clean up later or put this on hold until we have #111 sorted out.

@kraftner kraftner mentioned this pull request Nov 2, 2017
Merged
@moorscode
Copy link
Contributor

Let's add this functionality first and remove the testing from the plugin afterwards.
Could you resolve the conflicts?

@kraftner
Copy link
Contributor Author

I think it would be better the other way around. Because I'd need to fix the conflicts here, merge this, which then in turn will cause conflicts in #117 plus the need to rework everything in this PR anyway. Sounds like a lot of wasted effort.

Is there any reason you see with #117 why it can't be merged before?

@moorscode
Copy link
Contributor

@kraftner the only thing that was blocking #117 was a the bit of code that mocked the WordPress global & add_action functionality.

I have done a CR on that PR and provided a better way to resolve that problem.

@kraftner
Copy link
Contributor Author

Updated PR based on changed test data loading going into develop via #117

@moorscode
Copy link
Contributor

As #117 has been merged, this can be implemented.
Could you resolve the merge conflict?
Would be nice to also add some comment at each entry what data it is adding and why.

@moorscode
Copy link
Contributor

Could you also merge develop into this branch, to make sure all tests and travis are passing on it as well?

@moorscode moorscode removed the queue label Jan 3, 2018
@github-actions GitHub Actions
Copy link

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andizer andizer andizer left review comments

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

Successfully merging this pull request may close these issues.

Add test for post type without Yoast SEO Metabox
3 participants
@kraftner @andizer @moorscode