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

Update form styles #340

Merged
merged 5 commits into from Jun 3, 2013
Merged

Update form styles #340

merged 5 commits into from Jun 3, 2013

Conversation

dpoirier
Copy link
Member

forms.css has not been updated since Bootstrap was added. Some of these styles seem to be out of date.

@ghost ghost assigned dpoirier May 21, 2013
@dpoirier
Copy link
Member

I think we can get rid of forms.css.

I clicked around our contrib app pages that have forms and looked at what styles were applied (using Chrome Inspect Element) and didn't see anything from forms.css being used. I conclude that bootstrap is overriding everything we were setting in forms.css, so it's redundant.

There are some other stylesheets, like tables.css and modules.css, that still seem to be used. We could modify the contrib apps to work with pure bootstrap, but there could be other RapidSMS apps out there that rely on these styles, since apparently bootstrap isn't overriding everything there. So I think we should just keep those for now.

I clicked around our contrib app pages that have forms and looked at
what styles were applied (using Chrome Inspect Element) and didn't see
anything from forms.css being used. I conclude that bootstrap is
overriding everything we were setting in forms.css, so it's redundant.
@alexlemann
Copy link
Member

I noticed that the borders around forms go away, but that seems fine. I couldn't find templates using style from splits.css, modules.css, or icons.css. Did you find them used somewhere?

@dpoirier
Copy link
Member

Locations uses some of the styles from splits.css, icons.css, and modules.css.

I also worry that lots of existing apps that our users have written probably still use them. They probably haven't dived in headlong and re-done their styling for bootstrap yet.

@dpoirier
Copy link
Member

The consensus in our meeting Friday was to move these style files into the locations app and stop including them from the base template. If people are still using them, they'll have to include them themselves instead of getting them from the base template, but other apps won't be loading them by default anymore and then not using them.

- Stop loading old stylesheets in base RapidSMS template
- Move the old stylesheet files to the locations app
- Load the old stylesheets from the locations app's base template
- Modify the table styling in a few other contrib apps' code to take
  advantage of bootstrap
- Add a backwards incompatibility note to the 0.15 release notes.
@dpoirier
Copy link
Member

I moved the stylesheets to locations, and modified our other apps to add the style classes on their tables to trigger Bootstrap styling. @lemanal this is ready to review again.

@alexlemann
Copy link
Member

Might be nice to cleanup & get the module CSS & divs out of dashboard.html. Otherwise, this looks great.

@dpoirier
Copy link
Member

dpoirier commented Jun 3, 2013

I opened issue #401 to cleanup dashboard.html. I agree it would be good to do that.

@dpoirier dpoirier merged commit 5f357d8 into develop Jun 3, 2013
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.

None yet

2 participants