Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Modernize large parts of the UI #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

haveyaseen
Copy link

@haveyaseen haveyaseen commented Mar 18, 2020

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 visiting wrapper.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.

  • update jQuery to v3.4.1
  • update jQuery UI and theme to v1.12.1
  • remove ancient jQuery Selectbox plugin
  • self-host Oswald font (and all libraries)
  • 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

* 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>

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

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?

Choose a reason for hiding this comment

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

Copy link

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>

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

Copy link

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

Suggested change
</dl>
</ul>

Copy link

@justpetrovych justpetrovych left a comment

Choose a reason for hiding this comment

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

Nice work

@DanFlannel DanFlannel mentioned this pull request Mar 27, 2020
4 tasks
<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>
Copy link

@qk7b qk7b May 23, 2020

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

Suggested change
<a id="box-user-stats-link" href="#">(see stats)</a></span>
<span><a id="box-user-stats-link" href="#">(see stats)</a></span>

@qk7b
Copy link

qk7b commented May 23, 2020

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.

@tapper82
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants