Skip to content

Conversation

squirrelo
Copy link
Contributor

This adds the color change and the floating div.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.35% when pulling d258d12 on squirrelo:prettyfying into 2592d3e on biocore:cart-branch.

@squirrelo
Copy link
Contributor Author

This is ready to go

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.35% when pulling d258d12 on squirrelo:prettyfying into 2592d3e on biocore:cart-branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems rather strange that you have to have these two lines in the same line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't, but javascript doesn't care about line breaks and it saves some transmit info.

Copy link
Contributor

Choose a reason for hiding this comment

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

A byte? There are likely other larger areas of connection overhead that
would be more effective to worry about if this is a problem. We can do .min
versions later which would resolve extraneous whitespace, likely better to
worry about readability right now
On Apr 23, 2015 10:03 PM, "Joshua Shorenstein" notifications@github.com
wrote:

In qiita_pet/templates/list_studies.html
#1101 (comment):

@@ -38,7 +44,7 @@
moi.send('sel', proc_data);
}

-function show_alert(data) { bootstrapAlert(data + ' samples selected.', "success", 2000); }
+function show_alert(data) { bootstrapAlert(data + ' samples selected.', "success", 2000); $('#dflt-sel-info').css('color', 'rgb(0, 160, 0)');}

I don't, but javascript doesn't care about line breaks and it saves some
transmit info.


Reply to this email directly or view it on GitHub
https://github.com/biocore/qiita/pull/1101/files#r29022483.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, ok, I was lazy. Fixed.

@ElDeveloper
Copy link
Contributor

👍 this looks great, though a minor thing: I think it would look better on the right side of the screen, to hint at the fact that it refers to the "selected" menu.

@squirrelo
Copy link
Contributor Author

Issue with right side of screen is that the div is position:absolute, so I would need to dynamically adjust the x position with that. As is, I use x=0 so it's much easier and screen-size-independent.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.35% when pulling 610b9aa on squirrelo:prettyfying into 2592d3e on biocore:cart-branch.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.35% when pulling a77a391 on squirrelo:prettyfying into 2592d3e on biocore:cart-branch.

@antgonza
Copy link
Member

Some suggestions:

  • If a study/processed-data has already been selected either change
    the "Add" button or when adding change the message so it reflects that
    the study/processed-data was already selected and nothing changed
  • You need to add something that lets the user know that a study was
    already selected. In the past you had a checkmark. Perhaps combining
    this suggestion with the previous one will be a good idea
  • Can you add a clear button? Remove everything from the analysis.
  • In the expanded version there is a lot of space between "Processed
    Date" and "Algorithm", not sure why.

@squirrelo
Copy link
Contributor Author

In my opinion, your first two suggestions are going to confuse users. They are actually selecting the samples every time, it's just whether they are already registered or not on the back end that matters. Users will most likely not care about previously selected counts and all that, just that the ones they are currently selecting are registered.

@squirrelo
Copy link
Contributor Author

Took care of last 2 suggestions. Good to go again.

@antgonza
Copy link
Member

antgonza commented Apr 24, 2015 via email

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 79.31% when pulling b43291a on squirrelo:prettyfying into 2592d3e on biocore:cart-branch.

@squirrelo
Copy link
Contributor Author

So tehy look at the selected studies page?

@antgonza
Copy link
Member

antgonza commented Apr 24, 2015 via email

@squirrelo
Copy link
Contributor Author

Neither is what you are proposing. It's inconvenient and confusing.

@squirrelo
Copy link
Contributor Author

This is something we need to leave to user testing in my opinion. I think it's good as is and, if in user testing, it comes out as needed I will add it. This is the first alpha after all.

antgonza added a commit that referenced this pull request Apr 24, 2015
Pretty-fying sample cart and selection alerts
@antgonza antgonza merged commit 61166af into qiita-spots:cart-branch Apr 24, 2015
@josenavas
Copy link
Contributor

@squirrelo just a general comment in terms of the UI. Note that the first users of the interface are the reviewers. If the reviewers already find something that is confusing to them, is a read flag that the interface is going to be confusing to the users. Also you're basing your arguments on your brain image of the interface, while @antgonza is already seeing in your current interface that something does now work. Probably, his proposal is not the best one, but is already saying that there is something wrong with the interface. Check the new Contributing.md to see why this is important.

@antgonza given that it looks like there is no agreement on the final UI (although I agree with you that there is a problem), it would be useful if you can mock up something to show an improvement. In the past, I downloaded the final HTML from chrome and I did quick modifications there so I can see how the interface is going to look like (but is not functional). I think this will help to @squirrelo to understand that his UI is missing something.

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.

6 participants