-
Notifications
You must be signed in to change notification settings - Fork 697
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Make style of loading page similar to other pages #423
Conversation
Nice! This change might seem minor but it makes the critical rendering path more visually pleasing (mainly due to no more |
</div> | ||
</div> | ||
</div> | ||
</div> | ||
<div id="chat"></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.
Keeping it in #chat
meant it will be overridden once the chat has loaded, thus not keeping the loader in DOM. Any reason you moved it out?
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.
Indeed. I actually started by leaving it in there, but I was forced to cancel everything that the #chat
was setting, and it was a bit messy to hide the input field.
Making it its own window made more sense and cleaned up things a bit.
I hear you though, so I am now detaching the loading page in the init
event handler. Doing it earlier (when the # page appears) means I have to duplicate for public mode, so it's good like this I believe.
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.
Guess that makes sense, fine by me.
👍 except for the inline comment. |
3cd4388
to
01947a4
Compare
@@ -178,6 +178,7 @@ $(function() { | |||
} | |||
|
|||
$("body").removeClass("signed-out"); | |||
$("#loading").detach(); |
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 should have been .remove
, we don't be reusing the element at a later time.
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.
Indeed! Should I use .remove()
on $("#sign-in")
next line as well?
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.
Probably.
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.
Done in e5dddba.
- Move markup around to make the loading page a window of its own instead of a sub-window of `#chat` - Remove inline styling in loading page - Use same styling than other non-messages windows for title and text - Add a `z-index` to the loading page to hide the message input - Vertically align the # page title with all other titles - Make sure all `h1.title`s are bottom-margined consistently and remove negative margin on the Settings page title to align with the Connect page title (Reverting/Improving this should be done at the `h2` level instead)
01947a4
to
e5dddba
Compare
All of @xPaw's comments have been addressed :-) |
Just tried it out, looks good. And the code seems fine :-) I agree with keeping it outside of #chat, because it really shouldn't be in there anyway. 👍 and merging. |
…-styling Make style of loading page similar to other pages
#chat
z-index
to the loading page to hide the message inputh1.title
s are bottom-margined consistently and remove negative margin on the Settings page title to align with the Connect page title (Reverting/Improving this should be done at theh2
level instead)I can't admit or deny that the style of this is better or worse than the previous version. To me they are equivalent. However, this is now consistent with the # page, and all other non-chat pages.