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

#299 add toolbar for markdown formatting #305

Merged
merged 9 commits into from
Apr 15, 2019
Merged

Conversation

Mavrin
Copy link
Collaborator

@Mavrin Mavrin commented Apr 11, 2019

Solution for #299
It work the same way as it works on github.
Underhood we use package from github team.

How it looks:
Screen Shot 2019-04-12 at 10 34 56 PM
Screen Shot 2019-04-12 at 10 35 15 PM

@Mavrin Mavrin changed the title #299 add bold toolbar for formatting #299 add toolbar for markdown formatting Apr 12, 2019
@Mavrin Mavrin marked this pull request as ready for review April 12, 2019 19:41
@Mavrin
Copy link
Collaborator Author

Mavrin commented Apr 13, 2019

Could anyone review it? @Guria, @umputun, @igoradamenko

@Reeywhaar
Copy link
Collaborator

Why silence here? Good job. Though I don't like that you store icons as react components. Better store them as plain svg and take advantage of https://www.npmjs.com/package/svg-inline-react

@Mavrin
Copy link
Collaborator Author

Mavrin commented Apr 13, 2019

@Reeywhaar I see only one advantage, that you can open icon in some image viewer.
if I use real react component, I can change icon by props. e. g. size, className, fill.
And I don't need add dependencies.
I think for current case svg-inline-react is over-engineering

@Reeywhaar
Copy link
Collaborator

Yep, this way it can be not only opened in viewer, but edited in redactor, given to designer, reused in some other place, etc.

size, classname, and fill (via css) all can be set on top element. I understand that sometimes loading this way can be handy if svg is highly dynamic. But this is not the case, it just icon.

I think better to load images as require(!raw-loader!some-image.svg) and make tiny functional component

function SVG(props) {
return <span className={"icon "+props.className} dangerouslysetinnerhtml={{_html: props.svgstringorsomething }}></span>
}

or something. I agree that svg-inline-react can be overload, but there lot of alternatives, for example https://www.npmjs.com/package/react-svg-loader which is bundle time thing.

@Mavrin
Copy link
Collaborator Author

Mavrin commented Apr 13, 2019

How is often this project change icons? For me, It is not problem covert 8 raw icons to svg manually.
react-svg-loader looks better. But It need add one more loader.

@Reeywhaar Reeywhaar mentioned this pull request Apr 13, 2019
@umputun
Copy link
Owner

umputun commented Apr 13, 2019

with an extra toolbar the edit field looks too small. I'd suggest to make it 1 or even 2 lines higher

@Mavrin
Copy link
Collaborator Author

Mavrin commented Apr 14, 2019

4 lilines looks enough:
Screen Shot 2019-04-14 at 11 19 29 AM

5 lines:
Screen Shot 2019-04-14 at 11 21 46 AM

@umputun
Copy link
Owner

umputun commented Apr 14, 2019

4 lilines looks enough

agree

@Mavrin
Copy link
Collaborator Author

Mavrin commented Apr 14, 2019

Could you merge this request?

@umputun umputun merged commit 0585c32 into umputun:master Apr 15, 2019
@umputun
Copy link
Owner

umputun commented Apr 15, 2019

I have merged, and it is live on https://remark42.com/demo/

However, it looks kind of strange to me. This new toolbar makes the edit section visually unbalanced. Before the spaces on right, left and on the top were the same and it looked nice, but now it is not that nice anymore.

I'm not sure how to fix it, open for suggestions/ideas.

@Reeywhaar
Copy link
Collaborator

Not going far with design, but how about this? Considering mobile, and all things.
Screenshot 2019-04-15 at 07 00 06

@umputun
Copy link
Owner

umputun commented Apr 15, 2019

an interesting idea, but formatting toolbar on the bottom is unusual. Maybe the same but on top?

@Reeywhaar
Copy link
Collaborator

Same on top
Screenshot 2019-04-15 at 07 05 50

Unusual, but I think it's convenient as with long message you will have to drag cursor more to top than to bottom.

@umputun
Copy link
Owner

umputun commented Apr 15, 2019

still odd. I may have a better idea - how about removing the additional toolbar completeley and move it to the bottom instead of the current text, i.e.
xtdcv_20190414_234347 ryrw8

Subscription action can go to some new "subscribe" button (with a dropdown) and the same for MD help.

@Reeywhaar
Copy link
Collaborator

Unfortunately, on mobile it looks ugly then, conflicts with buttons on the left.

@umputun
Copy link
Owner

umputun commented Apr 15, 2019

maybe just collapse them all to a single button on mobile?

@umputun
Copy link
Owner

umputun commented Apr 15, 2019

another possibility is to hide this part on mobile, like github does

@Reeywhaar
Copy link
Collaborator

Reeywhaar commented Apr 15, 2019

Well, let's hide it then, I think burger button is too clumsy to use, and too close to "send button". But still I'm unsure about removing guide text, it contains useful info

@Reeywhaar
Copy link
Collaborator

Well, I tried, and it seems there is too many "if". I think buttons look like they unrelated to text area.

bad
Screenshot 2019-04-15 at 09 01 21
bad
Screenshot 2019-04-15 at 09 01 10
fine
Screenshot 2019-04-15 at 09 01 00
passes
Screenshot 2019-04-15 at 09 00 54

@Reeywhaar
Copy link
Collaborator

Hmm, I guess swapping is better.
Screenshot 2019-04-15 at 09 19 35
Screenshot 2019-04-15 at 09 19 50

@paskal paskal added this to the v1.3 milestone Jan 15, 2022
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

4 participants