Default UI for `/hoodie/account` #75

mbad0la · 2016-10-06T11:47:16Z

This is in reference to hoodiehq/camp#22

Work in Progress

gr2m · 2016-10-06T18:35:54Z

Tests are failing due to #3, you can ignore them.

gr2m

I like the clean, simple design, great starting point! Here is a preview for others

Some notes

Can we avoid alerts and instead show inline error messages or notifications?
When signing up, run sign in directly, do not ask user to sign in manually

gr2m · 2016-10-13T17:37:29Z

example/index.js

+    handler: function (request, reply) {
+      reply.file(path.join(__dirname, '..', 'public', 'client.js'))
+    }
+  })


Instead of defining separate paths for each static asset, let’s use inert to server all assets from the public/ folder at the / root path.

I'll use inert. Any hoodie repository already using inert? Might help me a lot.

Yes we use inert in the main hoodie repository to serve static assets from the Hoodie app’s public/ folder

instead of the three reply.file statements, you can do

server.route({ method: 'GET', path: '/{p*}', handler: { directory: { path: path.join(__dirname, '..', 'public'), listing: false, index: true } } })

and adjust the paths of <script src="/default/js/default.js"></script> and <link rel="stylesheet" href="/default/css/default.css"> to ./client.js and ./client.css

gr2m · 2016-10-13T17:42:17Z

public/index.html

-       Open browser console and play with <code>hoodie.account</code>.
-    </p>
+    <div class="generic-loader-wrap"></div>
+    <script src="/default/js/default.js"></script>


I would put all the HTML into index.html already, and just toggle it depending on the user’s state. Check out this account.html file from hoodie-app-tracker. It has a data-account-state="" property on the <body> tag, we set / change the state in common.js and we use the html attribute in CSS to toggle content in app.css

That helps a lot with a cleaner separation of content (HTML) and behavior (JS)

Hope that makes sense?

Hi @gr2m my current implementation is better is my opinion as it helps reduce the DOM by controlling DOM manipulations through JS.

HTML is JS has faced a lot of apprehension but it is gaining a lot of acceptance lately due to React with it's JSX Syntactic Sugar.

I seriously feel that it would be easier to maintain the code using my approach, although if you do think I should follow the approach given in hoodie-app-tracker, I'll do it.

Please let me know what you think is best. I'll do that.

A big objective for us is the accessibility and contributor-friendlyness of Hoodie’s code base. I agree that React and virtual DOM is fascinating, but it’s not solving problems that we have right now :) The simpler we can keep it, the better, so I’d rather like to follow the old-school approach and just use HTML and toggle content with JavaScript for now

gr2m · 2016-10-13T17:43:48Z

public/client.css

+  border-radius: 50%;
+}
+.generic-loader-wrap:before {
+  content: url(http://hoodiehq.github.io/hoodie-css/src/content_img/animals/low-profile-dog-1.png);


once we use inert (see comment below), let’s put this image into the public/ folder and load it with a relative path. We try avoiding loading assets from URLs to make sure that the apps work when run locally, even when not connected to the internet

@gr2m can you have a look at the html and js file now?
I've separated the html from the js file. Please comment so that I may continue finishing this up. Thanks!

varjmes · 2016-10-23T09:43:41Z

public/client.css

+  margin-left: -30px;
+  margin-top: -40px;
+  color: #FFF;
+}


Can we make sure each set of rules are separated with a linebreak please?

@Charlotteis sure! Must have missed it.

please add the empty lines and see my other comment. Otherwise it’s looking good 👍

varjmes · 2016-10-23T09:46:09Z

public/index.html

+
+    <div data-hide="false" class="generic-loader-wrap"></div>
+
+    <div data-hide="true" class="input-forms">


Rather than divs, which give no semantic indication of what the sections are, would section be better here for each of the 'Sign In', 'Sign Up, 'Sign Out', 'Update Account', 'Destroy' etc?

I don't follow how using section might be better here. Can you explain it a bit? I can always add an id attribute to each component in the UI.

These are all sections of content, so sections seems to make sense.
I try not to use div wherever possible as div is not very semantic HTML.

http://html5doctor.com/lets-talk-about-semantics/ might explain better.

a better question might be, why don't you think we should use them? :)

@Charlotteis semantically speaking, I think I get your point. I'll make the relevant changes soon.

gr2m · 2016-11-17T12:29:51Z

example/index.js

+    handler: function (request, reply) {
+      reply.file(path.join(__dirname, '..', 'public', 'client.js'))
+    }
+  })


instead of the three reply.file statements, you can do

server.route({ method: 'GET', path: '/{p*}', handler: { directory: { path: path.join(__dirname, '..', 'public'), listing: false, index: true } } })

and adjust the paths of <script src="/default/js/default.js"></script> and <link rel="stylesheet" href="/default/css/default.css"> to ./client.js and ./client.css

varjmes · 2016-11-19T11:28:55Z

public/index.html

@@ -3,11 +3,47 @@
  <head>


could we add lang="en" to the <html> tag above?

varjmes · 2016-11-19T11:30:17Z

public/index.html

+    <div data-hide="false" class="generic-loader-wrap"></div>
+
+    <section data-hide="true" class="input-forms">
+      <h3>Sign In</h3><br>


As the first header on the page, this should not be an h3.
Headings should not be used for decoration, they are important for content navigation.

So, the first heading on a page should be an H1.

Perhaps we can have an H1 heading of "Account" and then all the h3's can be h2 subheadings?

varjmes · 2016-11-19T11:31:27Z

public/index.html

+
+    <section data-hide="true" class="input-forms">
+      <h3>Sign In</h3><br>
+      <input name="login-username-field" type="text" placeholder="Username"><br>


Could we add labels to all form inputs, it's more accessible. Placeholders dissapear on focus.

mbad0la · 2016-11-23T16:38:29Z

@gr2m @Charlotteis please have a look at the new commits

gr2m · 2016-11-23T17:14:15Z

public/index.html

+      <label>USERNAME</label><br>
+      <input name="login-username-field" type="text"><br>
+      <label>PASSWORD</label><br>
+      <input name="login-password-field" type="password"><br>


two things:

avoid using uppercase only text because screen readers will read it out letter by letter, you can use css for that (text-transform: uppercase)

add the for attribute to labels and id attributes to the respective inputs, with same values, e.g.

<label for="login-username-field">Username</label><br> <input name="username" type="text" id="login-username-field"><br>

mbad0la · 2016-11-23T20:45:43Z

I think it's done now. Please let me know the same 😛

gr2m

looks good to me now, thanks! Ping @Charlotteis

varjmes · 2016-11-27T10:50:53Z

Great work @mbad0la!
This was a really good effort, thanks for bearing with us.
Approved!

gr2m · 2016-11-27T10:54:32Z

I will fix the CI errors and merge 👍

mbad0la · 2016-11-27T11:02:00Z

Great! Thanks for being so patient 🙂

gr2m · 2016-11-27T11:03:58Z

One thing that totally slipped in my review is that you used arrow functions in public/client.js, but as the code runs in browsers and we currently support browsers down to IE 10, we have to use good old es5 :) I fixed it now in
5c2c3c8

mbad0la · 2016-11-27T11:13:59Z

Oh, so sorry for that, didn't know about it and it has become kind of a reflex to write arrow functions :p

gr2m · 2016-11-27T11:16:46Z

I know, same here :) no worries at all

gr2m suggested changes Oct 13, 2016

View reviewed changes

varjmes suggested changes Oct 23, 2016

View reviewed changes

gr2m suggested changes Nov 17, 2016

View reviewed changes

varjmes suggested changes Nov 19, 2016

View reviewed changes

mbad0la added 9 commits November 23, 2016 19:57

Default assets routes + Default UI files init

b82c751

css asset + backbone ui logic

77c2444

Basic HTML Components

1cf2dad

Added actions for default UI

713f422

Default template base style

9072762

Separated html and js

7355ea2

Tag renamed to section as per semantics

01ffdef

Fixing improper conflict resolutions

74180f1

Wild card route for static resources + Inline messages

94ef6de

mbad0la force-pushed the default-ui-account branch from 01f7be0 to 94ef6de Compare November 23, 2016 16:10

Labels to input fields + Heading tags updated according to semantics

63a595d

gr2m suggested changes Nov 23, 2016

View reviewed changes

Minor semantic fixes

353f9cf

gr2m approved these changes Nov 23, 2016

View reviewed changes

varjmes approved these changes Nov 27, 2016

View reviewed changes

avoid arrow functions in code that runs in browser

5c2c3c8

gr2m merged commit 3b83a9a into hoodiehq-archive:master Nov 27, 2016

gr2m mentioned this pull request Jan 29, 2017

What could be a useful UI for /hoodie/account? hoodiehq/camp#22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default UI for `/hoodie/account` #75

Default UI for `/hoodie/account` #75

mbad0la commented Oct 6, 2016 •

edited

Loading

gr2m commented Oct 6, 2016

gr2m left a comment

gr2m Oct 13, 2016

mbad0la Oct 17, 2016

gr2m Oct 18, 2016

gr2m Nov 17, 2016

gr2m Oct 13, 2016

mbad0la Oct 17, 2016

gr2m Oct 18, 2016

mbad0la Oct 19, 2016

gr2m Oct 13, 2016

mbad0la Oct 23, 2016

varjmes Oct 23, 2016

mbad0la Oct 23, 2016

gr2m Nov 17, 2016

varjmes Oct 23, 2016

mbad0la Oct 23, 2016

varjmes Oct 23, 2016

mbad0la Nov 3, 2016

gr2m Nov 17, 2016

varjmes Nov 19, 2016

varjmes Nov 19, 2016

varjmes Nov 19, 2016

mbad0la commented Nov 23, 2016

gr2m Nov 23, 2016

mbad0la commented Nov 23, 2016

gr2m left a comment

varjmes commented Nov 27, 2016

gr2m commented Nov 27, 2016

mbad0la commented Nov 27, 2016

gr2m commented Nov 27, 2016

mbad0la commented Nov 27, 2016

gr2m commented Nov 27, 2016


		<div data-hide="false" class="generic-loader-wrap"></div>

		<div data-hide="true" class="input-forms">

Default UI for /hoodie/account #75

Default UI for /hoodie/account #75

Conversation

mbad0la commented Oct 6, 2016 • edited Loading

gr2m commented Oct 6, 2016

gr2m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbad0la commented Nov 23, 2016

Choose a reason for hiding this comment

mbad0la commented Nov 23, 2016

gr2m left a comment

Choose a reason for hiding this comment

varjmes commented Nov 27, 2016

gr2m commented Nov 27, 2016

mbad0la commented Nov 27, 2016

gr2m commented Nov 27, 2016

mbad0la commented Nov 27, 2016

gr2m commented Nov 27, 2016

Default UI for `/hoodie/account` #75

Default UI for `/hoodie/account` #75

mbad0la commented Oct 6, 2016 •

edited

Loading