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

Cosmetic fixes in the API browser HTML. #2187

Merged
merged 3 commits into from
Dec 8, 2014

Conversation

loic
Copy link
Contributor

@loic loic commented Dec 3, 2014

I had a quick go at this, just gathering some feedback, I can split into two PR if needed.

  • The tooltip issue is documented in the Bootstrap docs as "Tooltips & popovers in button groups require special setting":

screen shot 2014-12-03 at 5 10 33 pm

  • I naively assumed that the Bootstrap 2.x markup was simply forgotten but feel free to disregard if it was left there to remain compatible with user's overridden forms / templates:

screen shot 2014-12-03 at 5 11 22 pm
screen shot 2014-12-03 at 5 43 25 pm

@tomchristie
Copy link
Member

Looks good. Note the test failures - we evidently have some shoddy tests, but need fixing up regardless. Feel free to improve on them while your at it, or not, as you see fit.

@tomchristie tomchristie added this to the 3.0.1 Release milestone Dec 3, 2014
@loic
Copy link
Contributor Author

loic commented Dec 3, 2014

Cool thanks, I'll have a look at the tests.

Btw there are some CSS rules that don't play too well with Bootstrap: https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/static/rest_framework/css/default.css#L43-L45

Are these needed?

Also I got some pretty weird issue running the tutorial, ping on IRC when you get a chance ;)

@tomchristie
Copy link
Member

Are these needed?

Not obvious - you'd need to review that yourself.

@vccabral
Copy link
Contributor

vccabral commented Dec 7, 2014

@loic , tests are passing here if you want to consider if the tests are still valid.

https://github.com/vccabral/django-rest-framework/tree/cosmetic_fixes

@loic
Copy link
Contributor Author

loic commented Dec 8, 2014

Thanks @vccabral, I've added your commit to the PR. I wanted to take some time to review the other tests as Tom suggested but I've been rather caught up.

@vccabral
Copy link
Contributor

vccabral commented Dec 8, 2014

Np. Let me know if I can help @loic. Where are the other failing tests?

@loic
Copy link
Contributor Author

loic commented Dec 8, 2014

This build is passing so I think it's all good. It was more about reviewing and cleaning up the other tests.

@tomchristie
Copy link
Member

Fab, thanks @loic!

tomchristie added a commit that referenced this pull request Dec 8, 2014
Cosmetic fixes in the API browser HTML.
@tomchristie tomchristie merged commit b440bd7 into encode:master Dec 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants