-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
Multi Format Text Editor #126
Conversation
@@ -45,7 +45,7 @@ export { | |||
Panel, | |||
PanelMenuWrapper, | |||
Radio, | |||
RichTextEditor, | |||
MultiFormatTextEditor, |
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.
I'm happy with TextEditor
since we know we only have one anyway. But the longer name is 👌 to if you prefer it.
5c3b0bd
to
9617d60
Compare
88f0994
to
bb07cb6
Compare
279d246
to
127c03b
Compare
"fast-isnumeric": "^1.1.1", | ||
"immutable": "^3.8.2", |
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.
⚡
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.
i can't it's needed by draft-js..
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.
for real? Ugh ugh ugh. So to interop with this library you have to use a special wrapper around the regular data types because... Grrrr
Sometimes I want to become a farmer 👩🌾
"plotly.js": "^1.31.0", | ||
"prop-types": "^15.5.10", | ||
"react-color": "^2.13.8", | ||
"react-immutable-proptypes": "^2.1.0", |
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.
⚡
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.
i can't it's needed by draft-js.. same
@@ -188,7 +188,7 @@ class DefaultEditor extends Component { | |||
</Fold> | |||
<Fold name={_('Title and Fonts')}> | |||
<Section name={_('Title')}> | |||
<RichTextEditor /> | |||
<MultiFormatTextEditor attr="title" /> |
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.
you still like MultiFormatTextEditor? As this is our external API we can keep it as ring-a-ding as possible. But like I said if you love it then 👌
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.
yeah, sorry i still like it that way its clear for me we're not talking about the richTextEditor or something, TextEditor lacks some kind of keyword for me to indicate that its a super multi mega editor :)
<MultiFormatTextEditor | ||
value={this.props.fullValue} | ||
onChange={this.props.updatePlot} | ||
/> |
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.
oooh such simple API configuration I like it!
ref="textinput" | ||
value={this.state.value} | ||
rows={this.props.visibleRows} | ||
cols={this.props.areaWidth} |
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.
why does visibleRows
map to rows
but then areaWidth
map to cols
. Maybe TextArea should expose the same props rows
and cols
. That way MDN documentation will work for the wrapper component to
@@ -0,0 +1,58 @@ | |||
import React, {Component} from 'react'; | |||
|
|||
export default class TextArea extends Component { |
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.
Do we need TextArea at all? Does it need to be stateful? Does't Plotly.js gd.layout hold the state for us?
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.
well on the other hand if this is how it existed in the workspace I don't see a reason to make substantial architectural changes at this stage :)
// Reset the value to the graph's actual value | ||
if (nextProps.value !== this.state.value) { | ||
this.setState({ | ||
value: nextProps.value, |
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.
if state always equal to props.value why bother with state?
return ( | ||
<span> | ||
<textarea | ||
ref="textinput" |
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.
do we use that ref?
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.
yeah, its just a component that used to be there before, and i didn't pay as much attention to it, Per used it to create the html editor, I did the same and used it to extend the Latex editor too, i can try to remove, seems not that crucial indeed
@@ -65,10 +65,16 @@ | |||
"react": "React" | |||
}, | |||
"dependencies": { | |||
"draft-js": "^0.10.4", | |||
"draft-js-export-html": "plotly/draft-js-export-html", |
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.
nice work upgrading past our ancient forks!
@@ -439,3 +439,8 @@ export const SYMBOLS = [ | |||
{value: 'line-ne-open', alias: 43, label: 'M5,-5L-5,5', fill: 'none'}, | |||
{value: 'line-nw-open', alias: 44, label: 'M5,5L-5,-5', fill: 'none'}, | |||
]; | |||
|
|||
export const RETURN_KEY = 'Enter'; |
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.
A quick search seems to indicate that these don't really need to be in a constants file. They are only used in 1 file. Not critical but I am trying to keep a slim constants file under the general architectural philosophy "all the things you need in one place"
font-family: 'react-plotlyjs-editor'; | ||
src: url('data:font/truetype;charset=utf-8;base64,AAEAAAANAIAAAwBQRkZUTXjEJBIAAAqoAAAAHEdERUYANwAGAAAKiAAAACBPUy8yT9ZchwAAAVgAAABWY21hcAzoCWcAAAHMAAABSmdhc3D//wADAAAKgAAAAAhnbHlmH55HuAAAAzAAAASMaGVhZA2kwj8AAADcAAAANmhoZWED5QIFAAABFAAAACRobXR4Bx4BNAAAAbAAAAAcbG9jYQX6BJoAAAMYAAAAFm1heHAATwCBAAABOAAAACBuYW1ltk5aSgAAB7wAAAJJcG9zdKaWSGMAAAoIAAAAdQABAAAAAQAAzlDYZF8PPPUACwIAAAAAANZDQKEAAAAA1kNAoQAlACUB2wHbAAAACAACAAAAAAAAAAEAAAHbAAAALgIAAAAAAAHbAAEAAAAAAAAAAAAAAAAAAAAEAAEAAAAKAH4AAwAAAAAAAgAAAAEAAQAAAEAAAAAAAAAAAQIAAZAABQAIAUwBZgAAAEcBTAFmAAAA9QAZAIQAAAIABQkAAAAAAAAAAAABAAAAAAAAAAAAAAAAUGZFZABAAGEAZwHg/+AALgHb/9sAAAABAAAAAAAAAgAAAAAAAAACAAAAAgAAJQAlAG4AcQBuAIgAMwAAAAMAAAADAAAAHAABAAAAAABEAAMAAQAAABwABAAoAAAABgAEAAEAAgAAAGf//wAAAAAAYf//AAD/ogABAAAAAAAAAAABBgAAAQAAAAAAAAABAgAAAAIAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMEBQYHCAkAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAtgE8AVoBhAGkAdoCRgAAAAIAJQAlAdsB2wALAH0AACQ0JyYiBwYUFxYyPwEVFAcGDwEGBxYXFhQHBgcGIyIvAQYHBgcGKwEiJyY1JyYnBwYiJyYnJjU0Nz4BNz4BNyYvASInJj0BNDc2PwE2NyYnJjQ3Njc2MzIfATY3Njc2OwEyFxYVFxYXNzYyFxYXFhUUBw4BBw4BBxYfATIXFgFJFRY8FhUVFjwWpwIBBTQHBRMMAwMIFBYFBAMoCBIEBAEJQAUCAwgSCCgDCAMlCgICAQoDBAoCBwU0BQECAgEENQUHDxADAwgUFQYEAygIEgQEAQlABQIDCBIIKAMIAycIAgIBCgMECgIIBDQFAQLiPBYVFRY8FhUVUz8FAgICCBEJGA8DCAMKFRQDHwUGIxIIAgMDNQYEHgMDIg4CBQQCAg0EBA4DDBAIAwIFPwUCAgIIDgwUEwQGBAwSFQMfBQYjEggCAwM1BgQeAwMkDQEFBAICDQQEDwIODggDAgADACUAJQHbAdsAEwBIAFwAACU1NCcmKwEiBwYdARQXFjsBMjc2NzQmJyYjIgcGHwEWMzI3Njc2MzIWFRQHBgcGBwYdARQXFjsBMjc2NTQ3Njc2NzY3Njc2Nz4BFAcGBwYiJyYnJjQ3Njc2MhcWFwElAwMENgQDAwMEAzYDBANJIBgZF0YkBAYmAgMGAQsOCw0OFgYIDBMODwMDBDYEAwMGBgkIBgYHBgcFAwRtHR4yMHwwMh4dHR4yMHwwMh53NwQCAwMCBDcEAwICA8QYLAwLPQYGHAIEDgwHEAkLBggFCBEQEwsEAgMDAgQGCAkGBAQDBwQJCQkJFXwwMh4dHR4yMHwwMh4dHR4yAAAAAQBuAKUBkgFJAA8AAAAUDwEGIi8BJjQ3NjMhMhcBkgWABRAFgAUFBwYBAAYHAT0MB4AFBYAHDAcFBQAAAQBxAJ8BjwFFABgAAAEUDwEGIi8BJjU0PwE2MzIfATc2MzIfARYBjwOFAwgDhQMDDwIEAwRwcAQDBAIPAwEuBAOFAwOFAwQDAw8CAnFxAgIPAwAAAQBuAJIBkgE3ABEAACUUBwYjISInJjU0PwE2Mh8BFgGSBQYH/wAHBgUFgAYOBoAFpQgFBgYFCAcFgAYGgAUAAAEAiAB8AXgBhQAgAAAlFhQHBiIvAQcGIicmND8BJyY0NzYyHwE3NjIXFhQPARcBbwkJChgKREMKGAoICEdHCAgKGApDRAoYCgkJR0ewCRoJCAhOTggICBwIUFEIHAgICE5OCAgJGglRUAAAAAIAMwAzAc0BzQAhAEcAADc2MzIXFg8BBiImNTQ/ATY3NhcWFRQHBicmDwEGFBcWMjcTFhQPAQYjIicmNTQ3NjIXFj8BNjU0JyYnJg8BBiInJj8BNhcWF8oHCgsIDw8WHU46HUwkJSYcCAgSEhkrSw4ODiQO/B0dUSUoHxoHBwkSCRokUQ8PDBARDhoHFgcRERoaJyYciAcHEBQUHTomKB1MIwQEGggKCQoQEBorSw4mDA4OATodTh1RJRoHCgsIBwcaJlAOEhQMDQMDDhkHBxAUGRwCAh0AAAAADACWAAEAAAAAAAEAFQAsAAEAAAAAAAIADwBiAAEAAAAAAAMAMgDYAAEAAAAAAAQAFQE3AAEAAAAAAAUACwFlAAEAAAAAAAYAFQGdAAMAAQQJAAEAKgAAAAMAAQQJAAIAHgBCAAMAAQQJAAMAZAByAAMAAQQJAAQAKgELAAMAAQQJAAUAFgFNAAMAAQQJAAYAKgFxAHIAZQBhAGMAdAAtAHAAbABvAHQAbAB5AGoAcwAtAGUAZABpAHQAbwByAAByZWFjdC1wbG90bHlqcy1lZGl0b3IAAHAAbABvAHQAbAB5AGoAcwAtAGUAZABpAHQAbwByAABwbG90bHlqcy1lZGl0b3IAAEYAbwBuAHQARgBvAHIAZwBlACAAMgAuADAAIAA6ACAAcgBlAGEAYwB0AC0AcABsAG8AdABsAHkAagBzAC0AZQBkAGkAdABvAHIAIAA6ACAAMgA4AC0AMQAxAC0AMgAwADEANwAARm9udEZvcmdlIDIuMCA6IHJlYWN0LXBsb3RseWpzLWVkaXRvciA6IDI4LTExLTIwMTcAAHIAZQBhAGMAdAAtAHAAbABvAHQAbAB5AGoAcwAtAGUAZABpAHQAbwByAAByZWFjdC1wbG90bHlqcy1lZGl0b3IAAFYAZQByAHMAaQBvAG4AIAAxAC4AMAAAVmVyc2lvbiAxLjAAAHIAZQBhAGMAdAAtAHAAbABvAHQAbAB5AGoAcwAtAGUAZABpAHQAbwByAAByZWFjdC1wbG90bHlqcy1lZGl0b3IAAAAAAAIAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAACgAAAAEAAgECAQMBBAEFAQYBBwEIA2NvZw9xdWVzdGlvbi1jaXJjbGUKY2FyZXQtZG93bgphbmdsZS1kb3duCGNhcmV0LXVwBmNhbmNlbARsaW5rAAAAAAAAAf//AAIAAQAAAA4AAAAYAAAAAAACAAEAAwAJAAEABAAAAAIAAAAAAAEAAAAAzD2izwAAAADWQ0ChAAAAANZDQKE= | ||
') | ||
format('truetype'); |
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.
there was no newline before. Is prettier formatting your scss
as well or is this intentional?
@@ -1,18 +1,21 @@ | |||
.icon-cog:before { | |||
content: "\61"; | |||
content: '\61'; |
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.
is the scss being linted by your tools? That is probably ok but I haven't been linting scss like that.
$default-color-button-default: #119dff; | ||
$default-color-button-hover: #0f89df; | ||
$default-color-button-active: #0d76bf; | ||
$default-color-button-default: #119dff; |
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.
oh yeh I was trying to get rid of these and replace them with theme
names. These variables should be in _main.scss
not defaults.scss
🤷♀️
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.
later later
@@ -44,6 +44,9 @@ $color-link-light: $default-color-blue-pale; | |||
$color-panel-background: $default-color-white; | |||
$color-section-header-background: $default-color-white-blue; | |||
$color-fold-header-background: $default-color-rhino-dark; | |||
$color-accent: $default-color-dodger; | |||
$color-accent-shade: $default-color-dodger-shade; | |||
$color-button-hover: $default-color-button-hover; |
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.
I would like to ⚡ default-color-button-hover as that is a pretty terrible theme color name :) But the partition between theme vars and themable vars is incomplete
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.
later later
Super work. Double plus good for getting us off those 🍴 !! I'm a little hesitant on the inheritance based architecture as this repo has implemented composition everywhere we needed to extend a component. Also tests, what are we gonna do about tests? |
I'm open to tests in future PRs ... since I could use this code everywhere right now :) If you make an issue for tests I'll 💃 on this as is! |
No description provided.