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

sceditor css only in default theme #6837

Merged
merged 4 commits into from
Jul 17, 2021
Merged

sceditor css only in default theme #6837

merged 4 commits into from
Jul 17, 2021

Conversation

DiegoAndresCortes
Copy link
Member

For instance, the force_current is false by default so it wasn't needed in first place (unless I'm wrong of course)
And just load it from the default theme so themers don't have to update/maintain this file if it's ever updated.

For reference: https://www.simplemachines.org/community/index.php?topic=577856.0

In the topic I mentioned that this file wasn't copied when creating a new theme anyways, I didn't test if it's an expected behavior.

signed-off-by: diego andrés <diegoandres_cortes@outlook.com>
signed-off-by: diego andrés <diegoandres_cortes@outlook.com>
@@ -1516,7 +1516,7 @@ function create_control_richedit($editorOptions)
$context['drafts_autosave_frequency'] = empty($modSettings['drafts_autosave_frequency']) ? 60000 : $modSettings['drafts_autosave_frequency'] * 1000;

// This really has some WYSIWYG stuff.
loadCSSFile('jquery.sceditor.css', array('force_current' => false, 'validate' => true), 'smf_jquery_sceditor');
loadCSSFile('jquery.sceditor.css', array('default_theme' => true, 'validate' => true), 'smf_jquery_sceditor');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing 'force_current' => false is a good idea, but adding 'default_theme' => true is not. The jquery.sceditor.css file is used to style the editor toolbar, among other things, and I can easily imagine themes wanting the ability to style that. A dark theme, for example, might want to change the toolbar background to a darker colour.

Copy link
Member Author

@DiegoAndresCortes DiegoAndresCortes Jul 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we would just encourage to overwrite these styles using a custom file e.g. app.css or just in the index.css
This because the assumption of the sceditor requiring a css update in the future?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point. But if we are going to go that route, we should make it as easy and as obvious as possible to theme authors that they are supposed to do that. Something like this should do the job:

Suggested change
loadCSSFile('jquery.sceditor.css', array('default_theme' => true, 'validate' => true), 'smf_jquery_sceditor');
loadCSSFile('jquery.sceditor.css', array('default_theme' => true, 'validate' => true), 'smf_jquery_sceditor');
// THEME AUTHORS: If you want to change the CSS for the editor,
// include a 'jquery.sceditor.theme.css' file in your theme.
loadCSSFile('jquery.sceditor.theme.css', array('force_current' => true, 'validate' => true), 'smf_jquery_sceditor_theme');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.
We already have a beefy description in the index.template.php about loading files, should we just slap this note in there instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this is of course just for good measure, I would like for us to add this in the wiki in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just slap this note in there instead?

Why not both? 🙂

Signed-off-by: Diego Andrés <diegoandres_cortes@outlook.com>
Signed-off-by: Diego Andrés <diegoandres_cortes@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants