-
Notifications
You must be signed in to change notification settings - Fork 14
Modernize large parts of the UI #14
base: master
Are you sure you want to change the base?
Conversation
* update jQuery to v3.4.1 * update jQuery UI and theme to v1.12.1 * remove ancient jQuery Selectbox plugin * self-host Oswald font * update normalize.css to v8.0.1 * import Skeleton CSS library * use Skeleton's grid layout for initial responsive layout * communicate clickable areas as such using pointer cursor * homogenize styling of dynamic values for visual consistency * use logo file as background image instead of embedding it * format main.js using Prettier
</div> | ||
<div class="seven columns"> | ||
<div id="box-cause-selector"> | ||
<div class="panel-top-heading">I support research fighting...</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.
Could you explain please why do we need ellipsis here?
</li> | ||
</ul> | ||
</div> | ||
<p>My computer has <span id="box-status-time unknown">unknown time</span> to |
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.
As I see unknown
class wasn't hardcoded before. What is the reason for adding it now?
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.
Oh, I see.
You remove it in JS code 😃
https://github.com/FoldingAtHome/fah-web-client/pull/14/files?diff=split&w=1#diff-57086a2aa886379d4ee4c73304909162R477
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.
Is it normal to have this on an id
and not on a class
?
even when Web Control is closed. | ||
</p> | ||
<div id="box-project">Loading...</div> | ||
</dl> |
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.
Seems you have missed opening <dl>
tag
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 it is supposed to close the ul
tag line 159
</dl> | |
</ul> |
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.
Nice work
<div class="panel-top-heading">Points earned</div> | ||
<div> | ||
<span id="box-points-counter">Unknown</span> | ||
<a id="box-user-stats-link" href="#">(see stats)</a></span> |
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 see no opening tag for the span here
<a id="box-user-stats-link" href="#">(see stats)</a></span> | |
<span><a id="box-user-stats-link" href="#">(see stats)</a></span> |
Great job here ! May I suggest to add the alt attributes on the img tags that are used as buttons ? This will increase the app accessibility. |
@quentin7b Thanks for thinking about us screen reader users. I use NVDA if any testing is needed pleas give me a shout. I cant code but will install it on windows 10 and test with my screenreader. |
Hi, thought I'd put some work into this interface. Please excuse the huge size of this PR.
Unfortunately I couldn't yet figure out how to get the FAHClient to return the current
index.html
when visitingwrapper.html
so I wasn't able to test the changes using live data. Would appreciate some hints on how to do that if it's even possible.main.js
using Prettier