-
Notifications
You must be signed in to change notification settings - Fork 0
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
Ui changes steph #112
Ui changes steph #112
Conversation
I too, can get behind a gradual rolling out of ui changes. |
Oh boy new commits...trying them on now... |
I'm thinking I should just merge this. |
I just closed it, so that I can reopen it when it is actually ready. But if you want to merge what was there, feel free. |
I think this is good to go for now and should be merged. I am sure there is more improvement that can happen, but we should probably go ahead and merge it in. |
Do you want to resolve the conflicts or should I? |
Conflicts resolved! #conflictresolutionstrategies #growexperienceserve #reslife |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this look real good. Excited to have it be our new UI.
HOWEVER, there are few things that are currently broken. I tried to track them down to find the reason and I've commented as such below.
Checkout works pretty well as far as I’ve tested!
I haven't tested Swap Components yet since Input is not currently saving.
Phoenix/Views/RciInput/Index.cshtml
Outdated
@@ -1,8 +1,10 @@ | |||
@model Phoenix.Models.Rci | |||
|
|||
@{ | |||
ViewBag.Title = "Damage Input"; | |||
ViewBag.Title = "Damage Input: " + TempData["user"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work...think about when the RA is logged in and viewing someone else's RCI.
Solutions:
- Pass the name of the user from the controller, using a view bag.
- Use a view model :)
Phoenix/Scripts/rci-input.js
Outdated
|
||
|
||
/* Add a div to the right component. This div will contain a : | ||
<p> element displaying a damage | ||
<input> hidden element that will be used when submitting the form | ||
*/ | ||
function addDamage(componentID) { | ||
var pElement = "<p class='divAddOn-field new-damage'>" + $("#text-input-" + componentID).val() + "</p><i class='divAddOn-item material-icons' onclick='deleteNewDamages(event, this);'>delete</i>"; | ||
var pElement = "<p class=damage-element new-damage'>" + $("#text-input-" + componentID).val() + "</p><i class='material-icons' onclick='deleteNewDamages(event, this);'>close</i>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot a quote... consequence: damages don't save 🙅
@@ -0,0 +1,45 @@ | |||
@model Phoenix.Models.ViewModels.CheckoutRciViewModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add an index view page to RciCheckout 0_) ?
<input class="divAddOn-item adding-fines" type="number" placeholder="2.00" id="fine-amount-input-@component.RciComponentID" /> | ||
<button class="divAddOn-item" onclick="addFine(@component.RciComponentID);" id="add-@component.RciComponentID">Add</button> | ||
</div> | ||
<div class="div-list" id="div-list-@component.RciComponentID"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when stuff was moved around, the id got renamed?
It's supposed to be fine-list-@component.RciComponentID
I use it as a hook to know where to insert new fines. Currently nothing happens when you click Add
</ul> | ||
} | ||
|
||
<div class="view" data="@Model.GordonID"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably still have this <div>
tag wrapping everything. It contain a data
attribute which stores the person's ID which is used by rci-checkout.js.
|
||
<div> | ||
|
||
@if (Model.GordonID == ViewBag.GordonID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ViewBag.GordonID
isn't set anywhere... this displays the wrong overlay when the RA is logged in.
Steph said I could push directly to solve the issues outlined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second batch of reviews
EDIT: These have now been solved.
} | ||
|
||
function RASigSubmit() { | ||
var rciSig = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here. (This might be more of Rachel's error)
The RA never has three input fields to fill. Yet you get three values. This means that sometimes, depending on the situation, one or two of those values is going to be undefined. This isn't bad thing, except that it causes weird behavior in the SaveRASigs
method in the RciInput Service when the RA is signing his own rci.
The gist of this comment is that we need to make sure than when the rci belongs to the RA, he signs both the CheckinSigRA and CheckinSigRes fields. Right now it doesn't do that :/
} | ||
|
||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to include the check()
method for RDs that queues an rci to be signed 🙅♂️
The next commit is going to be simply styling tweaks, especially for mobile. |
Sweet, I think we good now.
|
Thanks Steph! |
Great!! Thanks for fixing it up Eze! I am glad you were able to find those
issues. 👍🏻
…On Sun, Mar 19, 2017 at 2:46 PM Eze Anyanwu ***@***.***> wrote:
Merged #112 <#112>.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#112 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AOo5WrAJnbvasH4zq2n3GRyTNu__qyeUks5rnaIlgaJpZM4MVLSr>
.
|
Here is the first round of UI changes.
I haven't touched the overlays for a signature, and I think swap components might look a little weird. But I have made a good number of changes in general, so I thought I would put this out in case you guys want to give feedback or merge stuff with master so that we can make sure the new code and functionality, which we are adding elsewhere, conforms to the new UI setup.
UPDATE: