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

MOTECH-2732 Migrated module install Workflow #39

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

shanmukhm
Copy link

  • Added a new directive for module installation form
  • Also migrated 'motech-file-upload' directive to 'motech-common'

@nickdotreid nickdotreid self-assigned this Jul 27, 2016
$scope.startOnUpload = function () {
if ($scope.startUpload !== true) {
$scope.startUpload = true;
$('.start-on-upload').find('i').removeClass("fa-square-o").addClass('fa-check-square-o');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the CSS manipulation from the controller and either do it in the markup or a directive link function?

@nickdotreid
Copy link
Contributor

@shanmukhm -- I'll admit I don't like either of the libraries you installed (and assume we were using them) malsup/form is pretty unsupported, so lets definitely drop that.

I don't love JansyForm, but it works and its supported — and you wrapped the dependency that uses it, so we can change it later if need be (I also like how it looks)

(1) There is a layout problem with smaller screens -- basically the submit button for the form is cut off by the screen (and we don't let people sideways scroll)
image

I think the simplest solution would be:
(a) change form-inline so it wraps
(b) add styles to make the file-input more space efficient (or wrap nicely)
(c) Change this form so it is in a modal window -- which would allow it to act like a normal form.

I personally like option (c) the best -- but I defer to you if you think making this implementation change is for the best.

@shanmukhm
Copy link
Author

@nickdotreid Actually I thought it should be exactly same as the old one. That is why I used the old ones. I should've informed you before installing new libraries.

I think I can try out option (c) and see if it works.

@jredlarski
Copy link
Collaborator

@shanmukhm I've added a motech-file-upload directive in a separate PR as it was required for both PRs so please use it and resolve any conflicts if they occur.

@motech-gerrit
Copy link

Can one of the admins verify this patch?

@shanmukhm
Copy link
Author

@nickdotreid I just have a small question. Should the form be an inline-form or a normal form?
If it is inline, can we change the width of the modal if we use Modal Window? I don't see any option for it.

@nickdotreid
Copy link
Contributor

@shanmukhm I think it will be more clear as a normal form

@shanmukhm shanmukhm force-pushed the motech-2732 branch 2 times, most recently from fb810be to fe8869e Compare August 1, 2016 14:14
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.

4 participants