-
Notifications
You must be signed in to change notification settings - Fork 101
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
Increase size of plain text editor #67
base: master
Are you sure you want to change the base?
Conversation
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 like the idea!
Does this also work for our other editors (medium / ckeditor)?
@@ -15,3 +14,6 @@ | |||
.page-list-item h3 a { | |||
display: block; | |||
} | |||
.page-simple-form-height { | |||
height: 400px !important; |
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 not a fan of !important
. Wouldn't it be better to target the form via its ID (#field-content
) in addition with a custom class?
Also, page-simple-form-height
as a name doesn't tell me much. I'd also prefer a pages
prefix for namespacing. How about pages-text-editor
?
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.
It has nothing to do with the other editors. In fact there the pixel heights are set and for the plain text editor no height is set which is what I am trying to improve.
I think !important here is justified as we are overriding the normal CKAN behaviour as we are using this form in a weird way. It might have been/be better overall to not use this macro to create the form.
With page vs. pages - I am following the convention in the existing CSS. Simple-form-height seems a lot more precise to me vs. text-editor (see comment above).
Regarding the first commit, that is me pulling in the latest ckanext-pages changes, but in a rather stupid way via. the GitHub UI.
Also, what about that first commit (c16ca58) in this PR? According to GitHub you merged changes from the change log, but that's not what the commit message says and I'm not sure how this is related to this PR... |
ping @mattfullerton |
@torfsen I've actually answered all the points above in the comment - would be nice to keep discussion on such a minor change to a minimum |
Pages' markdown editor for plain text editing is calling the same form that CKAN uses for small things. But as pages tend to be longer, some more space would be nice. This PR creates a new CSS class and passes it through to make the textarea larger.