Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

introduce minHeight prop #177

Closed
wants to merge 1 commit into from

Conversation

nikgraf
Copy link
Contributor

@nikgraf nikgraf commented Mar 8, 2016

To avoid selection issues it is important the the div with the attribute contenteditable is expanded. Even in the examples minHeight was set to the container which led these issues:

https://draftjs.slack.com/files/nikgraf/F0RA7K0KW/focus_test_1.mov

In the RichText example it was working properly due setting min-height in CSS via deep selectors. I feel like this is not obvious and an easy trap to fall into. Exposing a prop and using it in all the examples it might help …

@tasti
Copy link
Contributor

tasti commented Mar 8, 2016

What's the default when no minHeight is set?

Also, can you preserve the alphabetical ordering :)

@nikgraf
Copy link
Contributor Author

nikgraf commented Mar 8, 2016

@tasti: @hellendag & I just discussed it in the chat - singling out style props is probably not the best idea 😄

It would be great to either allow to overwrite the whole style attribute (to customize it like this) or provide a custom className via props to allow generating hash based classNames with tools like JSS or CSSModules.

Any preferences? Happy to update & change the PR

@tasti
Copy link
Contributor

tasti commented Mar 8, 2016

@nikgraf: oops, saw the convo right after posting this 😛. Let's continue it in slack

@nikgraf
Copy link
Contributor Author

nikgraf commented Mar 9, 2016

Here the conversation to provide some context:

hellendag [1:10 PM] 
Thanks :simple_smile:

[1:13] 
@nikgraf: I think the container shouldn't have any default spacing or height on it, so the editor should just occupy the container

[1:13] 
Mostly I think I probably just shouldn't have put spacing on the container at all in the examples :simple_smile:

[1:14] 
I think it should probably just be documented that the height should be applied to the content element, not the container. Or if we switch to a top-level inline style definition prop, clarify it within that.

nikgraf [1:30 PM] 
@hellendag: yeah, makes sense regarding singling out style props

[1:32] 
I have use-cases where the min-height for editors is different. Is there any way right now to pass in a classname or in-line styles to set a min-height on the editor?

[1:33] 
I could use global-styling with selectors, but then draftJS would be the only thing in the entire codebase that needs global styling with deep selectors. I would like to avoid that … :simple_smile: (edited)
1  

[1:34] 
With CSSModules or inline-styles the times of global styling are gone …

jsleonard [1:56 PM] 
@hellendag: can you pass styleName/className props into DraftEditor with {…this.props} and concatenate those extra styles/classes (maybe just classes?), be them CSS Modules with local names or otherwise?

[1:56] 
regarding @nikgraf’s request

nikgraf [2:08 PM] 
@tasti: just read your comment on the issue

[2:08] 
:simple_smile:

[2:09] 
how do you feel? should it be possible to either overwrite style or className?

[2:09] 
if yes, on which level? :simple_smile:

tasti [2:13 PM] 
@nikgraf: I'm trying to figure which elements are styled often. As @hellendag mentioned, only "public" classes should be overridden. If it's just the `public/DraftEditor/content`, a style overwrite shouldn't be that bad. But if people are digging lower into the elements (ex. `public/DraftEditorPlaceholder/root`), maybe we should think of solving these all at once :simple_smile:

jmaguirrei [2:14 PM] 
joined #general

tasti [2:16 PM] 
I like @jsleonard's idea of passing in a custom `className` and have it concatenate with the default

nikgraf [2:17 PM] 
@tasti: yep, that sounds good

jsleonard [2:17 PM] 
it’s something I’ve done in the past, worked well

tasti [2:18 PM] 
and then in your own CSS you can do something like:
.MyDraftEditor .public/DraftEditor/content {
  min-height: 100px;
}

nikgraf [2:19 PM] 
@tasti: that could work, although I believe it’s not so beginner friendly (edited)

tasti [2:22 PM] 
@nikgraf: I guess you're looking for something that's more baked into the `Editor`?

nikgraf [2:23 PM] 
as far as I understood React Native wen down the route to have props for lower level styles they want to expose

[2:23] 
https://github.com/callemall/material-ui/issues/1951#issuecomment-152776304

GitHub
Use of media queries, pseudo elements, hover, class names on inner elements       · Issue #1951 · callemall/material-ui · GitHub
Since the change to inline styles how am I supposed to: Add media queries so I can change the styles per breakpoint of material ui components. Add pseudo elements. Add hover affects. Override the...

[2:23] 
Like `menuStyle`

[2:24] 
In React in the Web people tend to still work more with classNames

[2:25] 
so ReactTransitionGroup allows to customize inner classnames

[2:25] 
https://facebook.github.io/react/docs/animation.html#custom-classes
Animation | React
A JavaScript library for building user interfaces (24KB)


[2:26] 
Personally I would be in favour of theme prop that takes classnames:

const theme = {
 rootClassname: ’some-classname’,
 contentClassname: ’this-is-my-content-classname’
}

<Editor theme={ theme} />

[2:27] 
but since everybody is doing it differently it’s really hard to say what’s best

[2:27] 
it depends a lot on the setup people have and their preferences

nikgraf [2:33 PM] 
@jsleonard’s solution is also solid & high up in my list of favourites. I think the important thing is to make it clear that the inner content must be expanded and it should be somehow flexible :simple_smile:

tasti [2:36 PM] 
I like that! Since there are multiple points of interest within the `Editor` that can be styled, passing in a `theme`-like object works well

@nikgraf
Copy link
Contributor Author

nikgraf commented Mar 31, 2016

I think we can open this up again as an issue … the PR is outdated

@nikgraf nikgraf closed this Mar 31, 2016
@nikgraf nikgraf deleted the min-height-prop branch March 31, 2016 23:20
@cancan101
Copy link

Did this ever get refiled?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants