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

feature(comments): Added separate edit page for generic comments.php #6715

Closed
wants to merge 1 commit into from

Conversation

Srokap
Copy link
Contributor

@Srokap Srokap commented Apr 13, 2014

That handles case when user opens edit link in new tab.

Fixes #6666

  • Cancel button doesn't work
  • Maybe forward to container on save?

@Srokap Srokap added this to the Elgg 1.9.0 Release Candidate milestone Apr 13, 2014
forward(REFERER);
}

$subject = $comment->getContainerEntity();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be careful with this word "subject". I initially thought you meant the grammatical subject as in the author or the person who wrote the comment, as opposed to the topic that was being commented on (i.e. blog, etc.).

I'd prefer "target" as that's what it is in the river now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, good that you mention it.

@ewinslow
Copy link
Contributor

LGTM, aside from the one minor variable-naming nit.

That handles case when user opens edit link in new tab.

Fixes Elgg#6666
@Srokap
Copy link
Contributor Author

Srokap commented Apr 14, 2014

Corrected and amended commit..

@mrclay
Copy link
Member

mrclay commented Apr 14, 2014

Two issues:

  1. After saving it leaves you back on the edit page; wouldn't it be more helpful to forward to the container?
  2. Do we really need to handle this (users middle-clicking on arbitrary UI elements)? This only opens a new window because we used a link, when really this should semantically be a button.

@ewinslow
Copy link
Contributor

If we only want this functionality available via JS, we need to actually make it a <button>. But that seems like its more hassle than its worth right now, no?

@Srokap
Copy link
Contributor Author

Srokap commented Apr 14, 2014

Ad. 1. Yes I don't like it either, but wanted to avoid messing with action as I will have to add some weird condition in action or add some additional parameter in the form. Also "cancel" link don't work

Ad. 2. I think it's nice practice that makes at least dev life easier in case of javascript crash. I wouldn't be surprised if it was a good accessibility practice. I'm trying to make it reuse as much as possible and keep it simple.

I'll see what can be done with minimal changes.

@ewinslow
Copy link
Contributor

@Srokap any updates on this?

@mrclay
Copy link
Member

mrclay commented Apr 19, 2014

See #6753.

@ewinslow ewinslow closed this Apr 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants