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

preventDefault on notifications #2528

Closed

Conversation

AgtLucas
Copy link

No description provided.

@ErisDS
Copy link
Member

ErisDS commented Mar 29, 2014

@AgtLucas without an explanation of why your change is needed, I have no way to verify this PR. Please read our contributing guidelines.

We're also in the process of completely rewriting the Ghost admin in Ember, so perhaps you'd like to get involved in that?

@AgtLucas
Copy link
Author

@ErisDS Sorry about that, was my mistake.

So, the reason for this PR it's because when you click to close one notification in admin, the URL get hash. I know that SEO in admin it's not important, and this sounds stupid but, IMHO, urls shouldn't get hash after you click in something.

I don't know why the Travis build failed.

About Ember, I read about that on dev blog, sounds really interesting, besides the fact I'm most familiar with Backbone and Angular, after reading #2144 I'm sure you guys did the best choice.

Thanks :)

@jgable
Copy link
Contributor

jgable commented Mar 29, 2014

I agree about preventing unnecessary hashes, but it does tend to be a personal taste thing. Part of the reason I propose switching to void(0) in #916.

Based on quick google search, Ember automatically inserts the event.preventDefault for actions. So this should be handled in Admin++.

@ErisDS
Copy link
Member

ErisDS commented Apr 7, 2014

Ember automatically does this for us, so even if I merge it it'll only be in the codebase a short while, which I would do except this doesn't meet our contributing guidelines

@ErisDS
Copy link
Member

ErisDS commented Apr 16, 2014

This'll get done in the Ember project.

@ErisDS ErisDS closed this Apr 16, 2014
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.

3 participants