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

Better action links for trashed posts #524

Merged
merged 11 commits into from
May 21, 2014
Merged

Better action links for trashed posts #524

merged 11 commits into from
May 21, 2014

Conversation

shadyvb
Copy link
Contributor

@shadyvb shadyvb commented May 12, 2014

Resolves #523

@shadyvb
Copy link
Contributor Author

shadyvb commented May 12, 2014

@lukecarbis Please review

if ( 'updated' == $record->action ) {
if ( $revision_id = wp_stream_get_meta( $record->ID, 'revision_id', true ) ) {
$links[ __( 'Revision', 'stream' ) ] = get_edit_post_link( $revision_id );
if ( $record->action !== 'trashed' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be yoda?
if ( 'trashed' !== $record->action )

Also what do you think about, testing for true here, instead of false?

if ( 'trashed' === $record->action ) {
    ...
} else {
    ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could even turn this into a switch:

switch( $record->action ) {
    case 'trashed':
        ...
        break;
    case 'updated':
        ...
    default:
        ...
        break;
}

Is that overkill?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukecarbis I was a switch guy till i knew that If/else is faster than switch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukecarbis About 'testing for true instead of false' i just thought I'd put the code that is more likely to happen first, sort of readable this way i think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like such a minor issue, but for some reason I've put a lot of thought into it. I think the most readable is a switch statement. It's also the most scalable (for example, if we added additional action conditions).

The speed difference is less than negligible. 0.046 millionths of a second according to the test on phpbench. I've never seen a length of time so small.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm open to being wrong. Any other opinions? @fjarrett @Japh @westonruter ?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as switch fall through statements aren't being used, then I don't really care. The case against fall-through cases is presented by Douglas Crockford:

switch Fall Through

The switch statement was modeled after the FORTRAN IV computed go to statement. Each case falls through into the next case unless you explicitly disrupt the flow.

Someone wrote to me once suggesting that JSLint should give a warning when a case falls through into another case. He pointed out that this is a very common source of errors, and it is a difficult error to see in the code. I answered that that was all true, but that the benefit of compactness obtained by falling through more than compensated for the chance of error.

The next day, he reported that there was an error in JSLint. It was misidentifying an error. I investigated, and it turned out that I had a case that was falling through. In that moment, I achieved enlightenment. I no longer use intentional fall throughs. That discipline makes it much easier to find the unintentional fall throughs.

The worst features of a language aren't the features that are obviously dangerous or useless. Those are easily avoided. The worst features are the attractive nuisances, the features that are both useful and dangerous.

http://oreilly.com/javascript/excerpts/javascript-good-parts/bad-parts.html

Copy link
Contributor

Choose a reason for hiding this comment

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

@westonruter Point well made. Let's keep it as is.

@lukecarbis
Copy link
Contributor

The problem here is that if you do restore a post, you still see the restore and delete permanently actions.

schermata 2014-05-14 alle 10 11 46 am

In this example, I restored the post (you can see the post updated at the top, that's me restoring it), but the restore option still displays.

@shadyvb
Copy link
Contributor Author

shadyvb commented May 14, 2014

@lukecarbis That's a good point, actually i did that on purpose, as the record is indicating the action of trashing a post, i thought it'd be logical if links reflect the action that happened on the record, rather than the current state of the post/object, eg: It'd not make sense to me if i see 'Restore' action link on a 'Post updated' record, if the post has been trashed recently, which is basically the same thing IMHO.

@lukecarbis
Copy link
Contributor

Yes, I see you point. At the same time, it doesn't make sense to offer a Restore action unless the link actually restores the post from the trash.

I think we should check against both conditions (stored post status was trashed and current post status is trashed) before we offer a restore link. Show nothing if the post has already been restored.

What do you think?

@frankiejarrett
Copy link
Contributor

@shadyvb I think @lukecarbis is right here. We should be showing action links based on the current state of the object, not relative to the individual record. It's misleading to the user if we display actions that are impossible to perform. I am working on changing this now.

@frankiejarrett
Copy link
Contributor

@lukecarbis Please review

@frankiejarrett
Copy link
Contributor

@lukecarbis OK done now for real, sorry 😄

@shadyvb
Copy link
Contributor Author

shadyvb commented May 20, 2014

@fjarrett What if we stop showing any action links if the state of the post has been changed ? I think it is misleading as well to have a 'Trash' post link for a 'Post x was trashed'.

@frankiejarrett
Copy link
Contributor

@shadyvb I'm not sure what you're talking about, there has never been a Trash action link.

If a post was trashed previously, but has since been restored, then you should see the actions available for the post's current state. And actually, it could be argued that this serves as an indicator that the post has since changed and to check newer records.

Action links are tricky indeed, but the simpler we can approach them, I think the better.

@shadyvb
Copy link
Contributor Author

shadyvb commented May 20, 2014

👍 And I was just trying to prove a point by the Trash example.

@frankiejarrett
Copy link
Contributor

Ah OK 👍

@lukecarbis
Copy link
Contributor

I'm still not happy with how this is working. Our requirements should be that action links relate to the summary, and that they can be successfully executed.

For example, if you restore a post, then trash it again, you currently see action links that look like this:
schermata 2014-05-21 alle 05 21 19 pm
(this doesn't make sense to me)

I would suggest that we show action links based on the saved state of the object only if the object currently has that same state. If it doesn't, don't show any action links.

@frankiejarrett
Copy link
Contributor

@lukecarbis Yeah I think we're all aware if this behavior, was just trying to keep it simple as this seems like a somewhat rare case.

But if you think it's a blocker then I'll keep working on it to make the logic more complex comparing the record to the current state of the object.

@frankiejarrett
Copy link
Contributor

@lukecarbis Please test the latest changes.

lukecarbis added a commit that referenced this pull request May 21, 2014
Better action links for trashed posts
@lukecarbis lukecarbis merged commit 6ad2015 into develop May 21, 2014
@lukecarbis lukecarbis deleted the issue-523 branch May 21, 2014 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Actions provided for trashed posts are irrelevant
4 participants