-
Notifications
You must be signed in to change notification settings - Fork 42
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
attempted a restyle on the chat interface + mic button #189
Conversation
I like the new my skills page, especially with the trash button. We need to make sure it's accessible though. |
views/devices_list.pug
Outdated
@@ -32,8 +33,9 @@ block content | |||
div.clearfix.visible-md | |||
div.col-lg-4.col-md-6 | |||
div.panel.panel-default.installed-dev | |||
a(href='https://thingpedia.stanford.edu/thingpedia/devices/by-id/' + dev.kind).panel-heading | |||
h2.panel-title= dev.name | |||
div#cont_head |
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.
Repeated ID?
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.
that has to be a class, fixed
views/error.pug
Outdated
p Sorry that did not work | ||
p= message | ||
a(href='javascript:history.back()').btn.btn-primary Go back | ||
div.sp_blk |
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.
These class names are not very readable, and also don't follow the existing snake-case convention. I would expand them to full words.
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.
ok
div.navbar-header | ||
button(class="navbar-toggle collapsed", data-toggle="collapse", data-target="#page-nav") | ||
span.sr-only Toggle navigation | ||
span.icon-bar | ||
span.icon-bar | ||
span.icon-bar | ||
span.navbar-brand.scroller.hidden-xs.hidden-sm Almond | ||
span.navbar-brand.scroller.hidden-xs.hidden-sm | ||
img.logo_top(src=Config.BASE_URL + '/images/almond.png') |
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.
There is something wrong with the logo when I tried in Firefox, it overflows the navbar.
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.
There is something wrong with the logo when I tried in Firefox, it overflows the navbar.
haha no, design choice, trying to be catchy, I kind like it that way
I like the new my skills page, especially with the trash button. We need to make sure it's accessible though.
do you mean usability accessibility?
I'm not sure what the config thing would point to? We don't have such a page yet, and for most skills there is nothing to configure.
it is for the one who need it, but mostly a mockup to show also the approach, for the moment could be avoided, but now we know how it will look like
I think the rounded corners are a bit too strong, especially in the grid view. Also I thought rounded corners were going out of fashion?
ok, less round ok, but round is not outdated (look down here in github "cancel" "comment"), rounder is more friendly
Small bug: the "Configure new skill" button is misaligned.
design choice, it is not the same "content", it lead to a different path than the skills box
Finally, I saw you changed the general style of .panel, which is a Bootstrap 3 panel component. We need to test in almond-cloud, which uses panels in more places (at the bottom of the front page for the suggested commands, in Thingpedia, maybe some other places I don't remember) to make sure everything looks good.
Actually never changed the base, only overrided, so that everything it can be reverted to the base components
Mostly the whole re-design is also a proposal, so to give a fresh view, but everything could be discussed and fixed together
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.
do you mean usability accessibility?
Yeah I mean accessibility (a11y) - as in, accessible to people with disabilities.
design choice, it is not the same "content", it lead to a different path than the skills box
It looks off to me. Maybe it's just me.
public/javascripts/conversation.js
Outdated
@@ -374,6 +375,7 @@ $(() => { | |||
function appendUserMessage(text) { | |||
container.append($('<span>').addClass('message message-text from-user') | |||
.text(text)); | |||
container.append('<div id="almond_thinking_container" ><div id="almond_thinking_txt"><p>Almond is thinking</p></div><div id="almond_thinking"><span></span><span></span><span></span><span></span><span></span><span></span><span></span></div></div>'); |
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 if you refresh the page: existing messages are resent by the server, and the spinner gets stuck.
Also, hardcoding the message here is problematic for a number of reasons, including translation.
I would do away with the message and entirely and only show the spinner, and also make it fewer dots. 3-5 should be enough.
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 if you refresh the page: existing messages are resent by the server, and the spinner gets stuck.
Noticed later, trying to figure out a fix for that
Also, hardcoding the message here is problematic for a number of reasons, including translation.
I know, that is a quick and dirty test, alredy implemented correctly
I would do away with the message and entirely and only show the spinner, and also make it fewer dots. 3-5 should be enough.
ok
Let's merge this and let's continue with the almond-cloud UI. |
this is a first attempt for test as main purpouse