YaleSTC / shifts

Application to easily track shifts, reports, and payforms for employees.
MIT License
23 stars 18 forks source link

Using bootstrap to replace superfish.js, thickbox.js and tooltip.js #273

Closed njlxyaoxinwei closed 9 years ago

njlxyaoxinwei commented 10 years ago

I have made working versions of superfish.js and thickbox.js in pull request #254 and #257, respectively and have included the source files in the vendor assets. However, tooltip.js remains almost completely dysfunctional (nb #144), and thickbox.js was last updated in 2007 and is really out-of-date: I had to circumvent some jQuery deprecations to get it to work. Although superfish is not out-of-date yet, the gem we were using was and I had to get rid of it and include the newest source file instead.

On the other hand, the popular Bootstrap front-end framework provides similar functionalities: NavBar and Dropdown menus of superfish.js, Modal windows of thickbox.js and tooltips/popovers of tooltip.js. We should consider switching to Bootstrap.

I created the branch bootstrap for this purpose. It has all my pull requests merged in but we can rebase if needed.

njlxyaoxinwei commented 10 years ago

I am using the gem bootstrap-sass. I did not choose twitter-bootstrap-rails because bootstrap-sass works better with SASS, and has the newest Bootstrap version (3.1.1), whereas the latter is still on Bootstrap version 2.3.2

njlxyaoxinwei commented 10 years ago

To avoid Bootstrap disturbing the original style too much, I took advantage of SASS and followed this suggestion on StackOverflow to namespace the Bootstrap style inside .bootstrap-styles.

There are still a few conflicts remaining. I fixed the .active class conflict with verify_import.css by using :not(), the negation CSS pseudo-class

njlxyaoxinwei commented 10 years ago

To achieve displaying submenus on hover, I included this JS into the vendor assets, and right now on the branch, the entire navigation bar is free of superfish. Superfish is also used in the vertial navbars but we can probably use .nav-stacked or vertical button groups in Bootstrap

njlxyaoxinwei commented 10 years ago

Right now the Thickbox Modal windows all have ajax content from other pages. Bootstrap modal windows have a remote option that probably can do what we need.

In Bootstrap, the remote content only provides the markup inside .modal-content div. The page with the trigger link/button has to contain the .modal, .modal-dialogue and .modal-contentdivs..

The nice thing about Bootstrap modals is that they come with events and can probably make it easier to write the JavaScript. With thickbox.js, it is hard to run javascript on the ajax loaded content.

With thickbox.js, Shifts now distinguishes rendering only modal content vs. one full page by a layout param, as noted in thickbox-compressed.js

jasonkliu commented 10 years ago

There are additional gems required as backports for using this version of bootstrap-sass with rails 3.2 - please see the readme at https://github.com/twbs/bootstrap-sass#rails-32x and f63d5f74487ceb2cc4d7f828d30b28e4f7daf0ad

Compare URL: Take the most recent commit on this branch and paste it here: https://github.com/YaleSTC/shifts/compare/59dac6704093c13e70b6f9c06d6761d1f2ca3550...bootstrap?w=1

jasonkliu commented 10 years ago

The header of the application is found in app/views/layouts/include/main_header.html.erb which calls _chooser.html.erb in the same directory. This file finds each of the department names and links to them. However, when there is only one department (as usual), it just links to the dashboard. The Shifts button on the left also does the same, so this functionality is redundant. Through troubleshooting with the Rails console, it seems that we only have one accessible department so we always have two links to the dashboard. After adding a second department, it seems that the functionality is correct. This issue is fixed by linking to the correct department if the user only has one department. 0463dbc6d36d2e9418dd89b7b2c2d8bafd4dff81

jasonkliu commented 10 years ago

Yes, this fixes #52. I'm still in the process of reworking bootstrap - converting all of the UI elements will take awhile.

mnquintana commented 10 years ago

I'm taking this one over - you can track my progress in finish_bootstrap (forgot to add the issue #)

shippy commented 10 years ago

We implemented Bootstrap with @njlxyaoxinwei for stickies and announcements. Left are data objects, links, and payform item popup for edit and delete

njlxyaoxinwei commented 10 years ago

I converted the data_object updates and added preventing double submission etc. However there is a consequence of this conversion to Bootstrap. In the old version we are using session[:mobile_param] (or mobile_device? helper) to check if the form is rendered with modal window or a full new page, since thickbox only works on non-mobile views. However, now that Bootstrap has responsive design, we should change our way of detecting whether the form is on modal window

njlxyaoxinwei commented 10 years ago

I now handle this issue by making the form non-ajax when modal is not used (layout!=false), so that default html redirecting is in effect

njlxyaoxinwei commented 10 years ago

Thickbox is now completely removed in branch modal_windows

shippy commented 10 years ago

Am I correct in understanding that the only remaining part of this issue that has yet to be addressed is the replacement of tooltip.js, which might also be causing #253 and #144?

njlxyaoxinwei commented 9 years ago

It turns out that tooltip.js is not so beyond salvaging after all. A lot of progress has been made toward fixing tooltip.js on branch for #335 !

shippy commented 9 years ago

Closed via #393.