Update Feature/451 notification visibility branch for #461 #482
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`home_url()` will only add a trailing slash to a site's home URL if a path is passed as an argument. If no path is passed, a page preview could be something like: http://wp.wsu.dev?page_id=1 And it would work fine in most cases. In a subdirectory multisite configuration, a page preview could be something like: http://wp.wsu.dev/site?page_id=1 This could be processed by WordPress as a lookup for a page named "site" rather than a site with that URL because no trailing slash exists. When adding query args to `home_url()`, we should tell it to use a slash to avoid confusion. See `_get_page_link()` for how core builds these in a way where a slash is always added.
fix get_beginning_of_week returning wrong results when given $date is not a start of the week fix get_beginning_of_week returning wrong results on DST switching
All I did was to replace the timesince function with the human_time_diff function from WordPress. This way, the time difference is automatically created.
Return early if notifications are disabled. Also check for an empty result from the database, rather than 1.
add_comment_meta’s 4th arg is false by default, so no need to declare.
Simplify most of the logic. Also remove the need for two different CSS classes as we can just default to once set of styles.
Default to the “sucess” state. Also have the text aligned left which seems to work better for cases where a lot of people were notified.
Use the success/green and failed/red colors from WP core.
`install()` is not run on VIP Go sites, so we must add the caps via filters instead.
…ase since the original PR
Documenting edit_flow.php
Improve i18n - use human_time_diff in Story Budget module.
…pdate(). This expects the HTML form to use the POST method.
…tepad-superglobals Changed $_REQUEST usage to $_POST usage in handle_notepad_update()
…get-post-form Notepad dashboard widget method changed to POST
Fix get_beginning_of_week
…s-notification-meta Display who was notified for an editorial comment
…270-comments-notification-meta Revert "Display who was notified for an editorial comment"
…52-feature/270-comments-notification-meta Revert "Revert "Display who was notified for an editorial comment""
…ated-time Displaying updated time instead of creation time in Notepad Dashboard Widget
Readme updates and version bump 0.8.3
…lity # Conflicts: # modules/notifications/lib/notifications.js
… users The jQuery used to get the text from the node next to the textbox (username) There is now an extra div around the checkbox, so this is broken. New jQuery gets the parent of the checkbox, then searches it's siblings for the displayname and uses that
|
This could cause some issues merging this way, but not really your fault with making this change given the current state of PRs. I agree with your comment in another ticket about creating a feature branch to get all these things merged together since they are so interdependent. Then you can push a PR against the branch. |
|
Thanks @WPprodigy So I'll wait for further instructions WRT all this I guess. cc/ @sboisvert |
|
Hi @jerclarke, thanks for your work, however, merging this in might be a huge ordeal which none of us needs to go through :) Do you mind opening a new PR and abandoning this one? I believe I've merged all the other work in master already. My plan is to release 0.9 this week. |
|
Working on this today, takes awhile just to remind myself what I was doing a month ago. Can't guarantee anything ready, that's a pretty tight deadline. |
|
@jerclarke no worries, we’re not releasing it this week, it was an
ambitious plan. Not going to shoot ourselves in the foot over a holiday
release :)
…On Fri, Dec 21, 2018 at 12:47 PM Jer Clarke ***@***.***> wrote:
Working on this today, takes awhile just to remind myself what I was doing
a month ago. Can't guarantee anything ready, that's a pretty tight deadline.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#482 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAcB9rQmBM6kde8Lmn4G4vsL_byadcJAks5u7R6ugaJpZM4YVsFv>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
For this I started with
feature/451-notification-visibilitybranch, merged inmaster, then fixed a small conflict and commited.I then added a commit with my fix described in a comment on #461
Now I'm trying to submit a PR. This is all part of #265
If this is wrong, please tell me what I should have done instead. Thank you.