Closed jlmitch5 closed 4 years ago
I believe this is ready for review @ngwese @tehn
rad!
what does the check button do on the second screen during update? does it cancel? the italic text seems redundant as well--- might be more informative to just say "please wait..."?
how difficult would it be to put the error list into a table? ie. repo on the left, reason on the right. again "update failed" is redundant.
and lastly, "already up to date" doesn't seem like a fail state. should it be hidden?
appreciate the feedback @tehn !
what does the check button do on the second screen during update? does it cancel? the italic text seems redundant as well--- might be more informative to just say "please wait..."?
👍 on the text changes. the checkbox just dismisses the modal (it's present as part of the confirmOnly={true}
prop). Theoretically, you could dismiss the modal, do other stuff, and then when the update requests are finished, the success/failure info modal would pop up. This seems to be consistent with other dialogs, and there's no real reason we need to block the interface (make the modal non-dismissable), so I think I'm fine with the ux.
So far as adding a cancel for in-flight update all...I immediately send a request to update all the projects once the user checks the confirmation modal, and so far as I can tell, there doesn't really seem to be a way to "stop" a request once it's been made. So it doesn't really make sense, unless I'm wrong about that.
how difficult would it be to put the error list into a table? ie. repo on the left, reason on the right. again "update failed" is redundant.
We could. It looks like the way ModalContent works, I don't necessarily need to pass a string, but could rather pass some markup. I can do it as an unordered list and follow the styling more or less from the project list.
and lastly, "already up to date" doesn't seem like a fail state. should it be hidden?
All I'm doing is displaying what comes from the api in this list in the format name: response.error
. So this is the response text returned from the api. We could change the string on the go side to rip out "update failed:"?
Similarly, the api returns already up to date as a failure (500 response). We could change this to a 200?
I don't know too much about the backend stuff but I imagine neither of those changes are very difficult.
Right now the UI for update all is consistent with how update single works, so I think we would want to change at the response level and keep the two consistent. I.e. kind of weird to have update all return already up to data as a "success" and update single return it as a "failure"
I am ambivalent on the response changes, @ngwese what do you think?
I realized a couple bugs. I messed up the styling of the project list on full screen widths, and I think I need to add the refresh functions to the failure callback for partially successful updates. i.e.:
// refresh project list
this.props.getProjectSummary();
this.props.refreshCodeDir();
I'll also make the string changes and attempt the table styling suggested by @tehn
I'll wait to hear from you about the api response changes before making updates there @ngwese
@tehn non-api changing updates finished
Ended up using <table>
instead of <ul>
. I did also fix an issue where the last item was getting left out of the last modal's display, and I sorted the info by script name.
thanks @jlmitch5 !! this is looking good. i'll defer to @ngwese for the backend
@jlmitch5 - will take a deeper look at this today, looks like a welcome improvement.
@jlmitch5 - first thanks so much for hammering this out. i'm sure folks will welcome it as a great step forward in usability.
i've reviewed all the changes, done some testing, and ultimately tried tweaking some of the styling (mostly just to simplify it). in the process i ended up refactoring the code which creates the final modal dialog content. the tweaked styling looks like this:
update summary just includes "Updated" and "Failed" sections
already installed text just says "installed" and the enabled button text is darker
@tehn - with respect to the "already up-to-date" being treated as a failure. i agree it isn't really a failure. i spent some time looking at how to best address that and it will require some refactoring in the backend go code and the frontend js API. the short version of the story is that the backend handler for project update
is actually mixed up with the GET method for querying projects. it is a bit of technical debt i apparently failed to address. for now i think it is best to get this change out and fix that in a second pass.
i'm going to move ahead with merging your changes along with the refactoring
aside: the ModalContent
class it definitely in need of a major overhaul. every time i try to use it i feel like i end up with spaghetti code.
agreed, this is fantastic!
Catching up, sweet! Thanks for taking it to the finish line.
fixes https://github.com/monome/maiden/issues/175
DONE
on the available packages list, change the install button to non-clickable, installed grey text
on the installed packages list, have an "update all" button at the top, which only has a single confirmation dialog after clicking "update all", then recursively goes through each installed package and tries to update.
This last modal will change the header to succeeded if no projects fail. If a combination of succeeded and failed updates happen it should show the succeeded projects in a list, then the failed ones in a list (with the reason, like shown in the screenshot above).