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

Full screen markdown editor #882

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@kunall17
Contributor

kunall17 commented Jul 16, 2017

Based on Boris compose PR,

  • Support some wrapping tags
  • Supports autocomplete of streams, people, emoji
  • Support list tags

Screencast:
ezgif-3-98e9d356be

Autocomplete list:
simulator screen shot 16-jul-2017 2 18 06 pm

@smarx

This comment has been minimized.

smarx commented Jul 16, 2017

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 1, 2017

Removed the Header tags now!

Supports live preview now :)

Screencast:
ezgif-1-cec59b295d

@borisyankov Any chance you can merge your compose PR sooner?

@kunall17 kunall17 changed the title from [WIP] Full screen markdown editor to Full screen markdown editor Aug 1, 2017

@zulipbot zulipbot added reviewed and removed needs review labels Aug 5, 2017

@borisyankov

Good work. Some comments added.

@@ -51,6 +51,10 @@ export default class MultilineInput extends PureComponent {
onContentSizeChange={this.handleOnContentSizeChange}
placeholder={placeholder}
textInputRef={textInputRef}
ref={component => {

This comment has been minimized.

@borisyankov

borisyankov Aug 5, 2017

Contributor

This is likely a mistake here.

@@ -128,7 +129,10 @@ class ComposeBox extends PureComponent {
<AutoCompleteView text={message} onAutocomplete={this.handleAutoComplete} />
<View style={[styles.composeBox, { height: totalHeight }]}>
<View style={componentStyles.bottom}>
<ComposeMenu />
<ComposeMenu
handleMessageChange={this.handleMessageChange}

This comment has been minimized.

@borisyankov

borisyankov Aug 5, 2017

Contributor

The ComposeMenu will be responsible for the actions calling and no need to pass this to it.

This comment has been minimized.

@kunall17

kunall17 Aug 7, 2017

Contributor

You mean I should wire up the redux in ComposeMenu, as atm it's not possible to call actions from ComposeMenu?

This comment has been minimized.

@borisyankov

borisyankov Aug 7, 2017

Contributor

Yep, actually, this will go to master soon.
I did implement it in #996

});
};
const applyWrapFormat = ({ getState, item, setState }: GetStateItemAndSetState) => {

This comment has been minimized.

@borisyankov

borisyankov Aug 5, 2017

Contributor

Extract this to a separate file.

});
};
const isStringWebLink = (text: string): boolean => {

This comment has been minimized.

@borisyankov

borisyankov Aug 5, 2017

Contributor

This one too.

return pattern.test(text);
};
const applyWebLinkFormat = ({ getState, item, setState }: GetStateItemAndSetState) => {

This comment has been minimized.

@borisyankov

borisyankov Aug 5, 2017

Contributor

... and this.

});
};
const applyListFormat = ({ getState, item, setState }: GetStateItemAndSetState) => {

This comment has been minimized.

@borisyankov

borisyankov Aug 5, 2017

Contributor

... and this...

} else if (selection.start === selection.end) {
newText = replaceBetween(text, selection, `\n ${item.prefix} `);
newSelection = { start: selection.start + 4, end: selection.start + 4 };
}

This comment has been minimized.

@borisyankov

borisyankov Aug 5, 2017

Contributor

Think why these are dependent on getState?
Can you make them work on text and whatever calls them manages state?

This comment has been minimized.

@kunall17

kunall17 Aug 7, 2017

Contributor

I'll have to pass on text and selection object otherwise, getState.
I thought passing text and selection would be less performant!

{ key: 'S', wrapper: '~~', onPress: applyWrapFormat, icon: 'format-strikethrough' },
{ key: 'C', wrapper: '`', onPress: applyWrapFormat, icon: 'code-tags' },
{ key: 'CC', wrapper: '```', onPress: applyWrapFormatNewLines, icon: 'code-braces' },
{ key: 'L', prefix: '-', onPress: applyListFormat, icon: 'format-list-bulleted' },

This comment has been minimized.

@borisyankov

borisyankov Aug 5, 2017

Contributor

Unit tests for all utility functions, please?

@zulipbot zulipbot added needs review and removed reviewed labels Aug 8, 2017

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 9, 2017

@borisyankov Updated with test's!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 9, 2017

Rebased and fix merge conflicts!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Aug 26, 2017

@borisyankov Updated, this will be safe to merge as it doesn't comes in b/w in any functioning of the app, and already it had one round of review!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Sep 20, 2017

@borisyankov Updated

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Sep 20, 2017

We will work on finishing this, once we release publicly the iOS and Android (hopefully very soon).
I am also holding on working on cool new features till then.
My idea was to pop the editors with 'Toggle compose tools' command (in addition to the 'topic' editor)

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Sep 22, 2017

@borisyankov Okay, sounds good 👍

@zulipbot

This comment has been minimized.

Member

zulipbot commented Oct 8, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Oct 31, 2017

Found some time in my busy schedule of Lab exams + end semester exams on my head, so updated this to go with the latest awesome update!

@borisyankov rebased this to master!

@zulipbot

This comment has been minimized.

Member

zulipbot commented Nov 19, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Dec 1, 2017

Updated

@zulipbot

This comment has been minimized.

Member

zulipbot commented Dec 4, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Dec 4, 2017

A lot of the code of the PR is now OK, but also there is a lot to comment on... which will drag on... and you know why... because you did combine several features in one PR.

There are three features here:

  • preview (great one)
  • edit tools (good one, needs some polish)
  • new screen (somewhat useful, maybe not that much anymore)

Focus on a PR that includes only the preview first. Then we will merge it quickly (after a review) and move on to the second one.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Dec 4, 2017

Ok, will split out the PR into parts!

Though I believe a full-screen text editor is required for ease of markdown and writing big messages!

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Dec 6, 2017

Closing, to be posted in multiple PR's!

@kunall17 kunall17 closed this Dec 6, 2017

@zulipbot zulipbot removed the needs review label Dec 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment