Skip to content
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

chat: Interface redesign #1976

Merged
merged 7 commits into from Dec 1, 2019
Merged

chat: Interface redesign #1976

merged 7 commits into from Dec 1, 2019

Conversation

matildepark
Copy link
Contributor

@matildepark matildepark commented Nov 21, 2019

This PR implements the latest redesign of the Chat interface and adds additional features for mobile and PWA use, easier join flows, and a general refresh of the Indigo design components used in Chat.

Screen Shot 2019-11-20 at 8 15 13 PM

full

Features

  • Implements a new design by @urcades.
  • Datestamp now uses @d format.
  • Adds the ability to collapse and expand the sidebar.
  • Adds the ability to “popout” chats, removing the sidebar and Landscape chrome in a new window.
  • Adds a fully functional mobile layout, including a web app manifest for progressive web application compatibility and a “native app” look.
  • Adds the ability to create join links to chats by mentioning them by name (ie. saying “~dopzod/urbit-help” becomes a link to automatically join it).

Additional comments

  • Replaces tachyons with indigo-static. indigo-react should be gradually integrated as a next milestone.

Added exports so that indigo-react can
be integrated into our workflow without error.
@vvisigoth
Copy link
Contributor

vvisigoth commented Nov 21, 2019 via email

@Fang-
Copy link
Member

Fang- commented Nov 21, 2019

including a web app manifest for progressive web application compatibility and a “native app” look.

Absolutely amazing!

-1 for the floating create/join buttons (also looks like there's no bottom-padding/margin accounting for them, obscuring the last item), but otherwise looks very good! Fancy blue sigils.

@matildepark
Copy link
Contributor Author

Gotta tweak the buttons (I think they're 3px off center) and the input box in read-only situations, standby.

This commit redesigns the front-end of chat-view for
Landscape, adding a collapsable sidebar, popout chats,
a streamlined join flow, and a general refresh of the Indigo
interface.
@matildepark
Copy link
Contributor Author

OK cool, good for review.

@vvisigoth
Copy link
Contributor

@loganallenc plz review

matildepark and others added 3 commits November 22, 2019 20:11
Adding some very small viewport adjustments to keep the
layout tidy, disabling iOS autocorrect.
Lining up the headers for these two panels on various viewports.
Adds a blank white placeholder icon, so that it looks better
than a blurry screencap on mobile devices.
@matildepark
Copy link
Contributor Author

@vvisigoth found a bug in the regex I wrote for the join flow. Gonna patch that today.

This commit amends the join flow based upon
regular expression matching of chatroom names
to only match if the entire message is the chat.
@matildepark
Copy link
Contributor Author

Ok, done.

@tacryt-socryp
Copy link
Contributor

I will get to this as soon as I open my static gall PR. Thanks!

});
return;
}
this.props.api.chatView.join(ship, station);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice piece of additional functionality.

Copy link
Contributor

@tacryt-socryp tacryt-socryp left a comment

Choose a reason for hiding this comment

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

I still need to review the functionality of this, but it looks good.

{
sigil({
<div style={{ flexBasis: 32, backgroundColor: props.color }}>
{sigil({
Copy link
Contributor

Choose a reason for hiding this comment

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

good addition, will make contacts integration simpler

if ((chatroom !== null) // matched possible chatroom
&& (chatroom[1].length > 2) // possible ship?
&& (urbitOb.isValidPatp(chatroom[1]) // valid patp?
&& (chatroom[0] === letter.text))) { // entire message is room name?
Copy link
Contributor

Choose a reason for hiding this comment

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

oof. good, but unwieldy. we really need a better way to send around links in an ongoing fashion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regexing a message for valid room names that can now have any number of hyphens and slashes without catching stack traces and URLs is surprisingly annoying.

pkg/interface/chat/src/js/components/new.js Outdated Show resolved Hide resolved
pkg/interface/chat/src/js/components/new.js Outdated Show resolved Hide resolved
<a onClick={this.deleteChat.bind(this)}
className="pointer btn-font underline nice-red">{buttonText}</a>
<div>
<div className={"w-100 fl mt3 " + ((chatOwner) ? 'o-30' : '')}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why display this at all if it's not an available option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Insisted in the mockups and conversations after — I imagine you want people to see what the owner can do and infer how chatrooms in general work.

pkg/interface/chat/src/js/components/settings.js Outdated Show resolved Hide resolved
pkg/interface/chat/src/js/components/member.js Outdated Show resolved Hide resolved
pkg/interface/chat/src/js/components/member.js Outdated Show resolved Hide resolved
pkg/interface/chat/src/js/components/settings.js Outdated Show resolved Hide resolved
pkg/interface/chat/src/js/components/settings.js Outdated Show resolved Hide resolved
@tacryt-socryp
Copy link
Contributor

Layout issue upon creation of an empty channel in Brave.
Screen Shot 2019-11-27 at 5 01 30 PM

@tacryt-socryp
Copy link
Contributor

Also noticing that the Create new and join buttons are not noticeable when the chat list is empty

@tacryt-socryp
Copy link
Contributor

Chat input doesn't seem to auto-focus.

@matildepark
Copy link
Contributor Author

Autofocus was removed because on mobile that causes each and every page to jump to the textbox. If you know how to responsively enable autofocus, let me know.

@tacryt-socryp
Copy link
Contributor

On members page, the "Ban" button looks exactly the same font weight / style as the Host text, which is not a button.

@tacryt-socryp
Copy link
Contributor

Deleting a chat crashes the browser window.
Screen Shot 2019-11-27 at 5 04 14 PM

@matildepark
Copy link
Contributor Author

The layout issue you're talking about is that overflow-x-scroll produces a scrollbar in your browser — I noticed you saw scrollbars where I don't on urbit.org, too. I will just responsively enable it. We only want to overflow-x-scroll on mobile anyhow.

@matildepark
Copy link
Contributor Author

Deleting crash is from trying to access a variable set from a prop before initialisation. I'll do the component and fake API call fixes first before figuring out the life cycle stuff.

@matildepark
Copy link
Contributor Author

Deleting issue fixed. const instead of allocating it with let every time the component's mounted.

@matildepark
Copy link
Contributor Author

I believe I've addressed everything requested. The remainder is design criticism, which I think is outside the scope of the PR, as this matches the mockups that have gone through their own extensive reviews.

Changes to the structure stylistically during code review.

Using fake API calls instead of directly calling store;
fixing a 'delete' crash';
making common code a component;
autofocusing responsively.
@matildepark
Copy link
Contributor Author

This has been tested on release candidate and the following are confirmed functional with the API:

  • text message
  • URL message
  • Hoon message (with caveat that a broken Hoon message will stack trace in Dojo but not on front-end, leaving it pending)
  • leaving a channel and rejoining
  • revoking write access and visiting that chat
  • returning write access and using that chat
  • revoking read access and visiting that chat (with a caveat that it provides a stack trace in Dojo to the visitor trying to pull the backlog it isn't permissioned to)
  • returning read access and visiting that chat (with a caveat that messages sent in the interim period during the ban will not show up when you rejoin)

Copy link
Contributor

@tacryt-socryp tacryt-socryp left a comment

Choose a reason for hiding this comment

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

Has been tested against Philip’s rc branch and master. Great work Matilde.

@tacryt-socryp
Copy link
Contributor

Btw showing the stack trace when failing a subscribe is good!

@tacryt-socryp
Copy link
Contributor

Very righteous

@matildepark matildepark changed the base branch from master to rc November 28, 2019 04:29
@matildepark
Copy link
Contributor Author

@philipcmonk

@philipcmonk
Copy link
Contributor

Great, I'll merge this soon and try deploying it OTA on the testnet.

@philipcmonk philipcmonk merged commit b8a2d06 into rc Dec 1, 2019
@philipcmonk
Copy link
Contributor

I pushed this to the testnet OTA and everything seems fine.

@jtobin jtobin deleted the mp-chat-redesign branch December 4, 2019 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants