Skip to content

Conversation

@mattfullerton
Copy link
Contributor

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.

Copy link
Contributor

@torfsen torfsen left a 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)?

display: block;
}
.page-simple-form-height {
height: 400px !important;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@torfsen
Copy link
Contributor

torfsen commented Oct 4, 2017

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...

@torfsen
Copy link
Contributor

torfsen commented Jan 24, 2018

ping @mattfullerton

@mattfullerton
Copy link
Contributor Author

@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

@torfsen torfsen mentioned this pull request Feb 5, 2018
@EricSoroos EricSoroos deleted the feature-bigger-plain-editor branch October 5, 2020 11:26
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.

2 participants