-
Notifications
You must be signed in to change notification settings - Fork 156
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 categories to pull screen - 1 #1237
base: develop
Are you sure you want to change the base?
Conversation
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.
@kirtangajjar Thanks for the PR.
Targeted branch
I noticed the PR branch was targeting the trunk
due to which the workflows like PHPCS did not trigger. I updated the target branch to be develop
.
Testing
While testing I noticed the category link is broken because a '/' is missing:
Actual requirement
As per the requirement,
make the plugin very useful is if it was possible to view the category of the posts and select which categories to pull.
It seems like it says a user should be able to select categories but this PR only displays them. I'm not sure how do we achieve pulling posts via selected categories in the same UI. But is displaying them enough to should we worry about selection as well? cc @jeffpaul
E2E failing
Can you please check and fix the E2E failures? Try upgrading the wp-env
version.
Suggestions
I have also added a few suggestions below, mostly related to formatting.
Co-authored-by: Faisal Alvi <[email protected]>
This should be fixed now.
That will happen in another PR. I'll ping that PR here when it's ready. |
Which version of wp-env works fine? |
@faisal-alvi I worked fixing tests for a while and I was able to fix many things, but it looks like it'll require more work. Work done so far: https://github.com/10up/distributor/pull/1237/files#diff-4c9c322e893113577fd895f8ac7bf59b02ac95aaae52b64177f24a996f706c22 Work remaining:
Here we need to change
to
That command is coming from here - https://github.com/10up/cypress-wp-utils/blob/develop/src/commands/wp-cli.ts#L21. I think we should create a separate issue to fixing build pipeline given the work it's requiring so it can be completed in parallel to this 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.
@kirtangajjar thanks for the fixes, just a couple of minor suggestions below.
I think we should create a separate issue for fixing the build pipeline given the work it requires so it can be completed in parallel to this PR.
Agree, please create a separate ticket and we can put this ticket on hold until E2Es are fixed in the other PR, just to confirm that E2Es are not failing due to this PR.
@faisal-alvi I've resolved all your comments. LMK there is any additional feedback. |
@kirtangajjar Thanks for the update. Let's wait on the review/merge until the E2E tests are fixed in #1242 and merged. After that, we'll merge |
@kirtangajjar can you please resolve the conflicts? |
@faisal-alvi Strange, I don't see any conflicts. Only some flakey tests are failing |
@kirtangajjar there's a syntax error in the oembed test. Can you have a look at that?
|
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.
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.
@kirtangajjar Thanks for the updates. I tested the changes, and they work well with external connections. However, I noticed incorrect category links on internal sites.
Here’s the issue:
Main Site: When pulling posts from Sub Site 1, the categories link back to the Main Site instead of Sub Site 1.
Sub Site 1: When pulling posts from Sub Site 2, the categories link to Sub Site 1 instead of Sub Site 2.
* @since 0.8 | ||
* @since 2.0.5 |
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 is the version bumped for this existing function?
Description of the Change
Add categories to the pull screen. This is a part 1 of 2 of this issue. This PR only lists categories from posts.
Closes #428
How to test the Change
Changelog Entry
Credits
Props @kirtangajjar
Checklist: