Skip to content

Conversation

@jlmitch5
Copy link
Collaborator

@jlmitch5 jlmitch5 commented Jan 19, 2020

fixes #175

DONE

on the available packages list, change the install button to non-clickable, installed grey text

Screen Shot 2020-01-18 at 10 32 54 PM


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.

Screen Shot 2020-01-19 at 1 55 31 PM

Screen Shot 2020-01-19 at 1 55 37 PM

Screen Shot 2020-01-19 at 2 12 08 PM

Screen Shot 2020-01-19 at 2 11 52 PM

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).

@jlmitch5 jlmitch5 force-pushed the projectUXEnhancements branch from 578852b to 457e8db Compare January 19, 2020 19:20
@jlmitch5 jlmitch5 changed the title WIP: Project ux enhancements Project ux enhancements Jan 19, 2020
@jlmitch5
Copy link
Collaborator Author

I believe this is ready for review @ngwese @tehn

@jlmitch5 jlmitch5 force-pushed the projectUXEnhancements branch from 457e8db to dd9ab56 Compare January 19, 2020 19:25
@tehn
Copy link
Member

tehn commented Jan 19, 2020

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?

@jlmitch5
Copy link
Collaborator Author

jlmitch5 commented Jan 19, 2020

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

@jlmitch5
Copy link
Collaborator Author

jlmitch5 commented Jan 20, 2020

@tehn non-api changing updates finished

Screen Shot 2020-01-19 at 8 12 23 PM
Screen Shot 2020-01-19 at 7 43 03 PM


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.

@tehn
Copy link
Member

tehn commented Jan 20, 2020

thanks @jlmitch5 !! this is looking good. i'll defer to @ngwese for the backend

@ngwese
Copy link
Member

ngwese commented Jan 20, 2020

@jlmitch5 - will take a deeper look at this today, looks like a welcome improvement.

<tr>
<th colspan="2">The table header</th>
</tr>
</thead>
Copy link
Member

Choose a reason for hiding this comment

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

@jlmitch5 - this stray table markup seems like a left over from some earlier work, correct?

@ngwese
Copy link
Member

ngwese commented Jan 20, 2020

@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
Screen Shot 2020-01-20 at 1 17 56 PM

already installed text just says "installed" and the enabled button text is darker
Screen Shot 2020-01-20 at 1 19 31 PM

@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.

@tehn
Copy link
Member

tehn commented Jan 20, 2020

agreed, this is fantastic!

@ngwese ngwese merged commit 2742c95 into monome:master Jan 20, 2020
@jlmitch5
Copy link
Collaborator Author

Catching up, sweet! Thanks for taking it to the finish line.

@ngwese
Copy link
Member

ngwese commented Jan 21, 2020

...if you'd like to enjoy the fruits of your labor you can install the v1.0.1 release.

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.

ux enhancements to package install and update

3 participants