WordPress.org

Make WordPress Themes

Opened 5 weeks ago

Last modified 2 days ago

#46546 reviewing theme

THEME: Reykjavik – 0.9.11

Reported by: webmandesign Owned by:
Priority: previously reviewed Keywords: theme-reykjavik accessibility-ready
Cc: webmandesigneu@…

Description

Reykjavik - 0.9.5

Reykjavik is fresh, lightweight, speed and SEO optimized, accessibility ready WordPress theme perfect for your next business, portfolio, blog or WooCommerce e-shop website. You can customize all elements of the theme to your needs. The theme works perfectly with Beaver Builder, Beaver Themer, Elementor, Visual Composer and other page builders to create fantastic layouts. It features mobile-optimized codebase and design with unique, easy-to-access mobile navigation. Build your website in no time with integrated one-click demo import functionality. Impress your website visitors with this beautiful theme now!

Theme URL - https://www.webmandesign.eu/portfolio/reykjavik-wordpress-theme/
Author URL - https://www.webmandesign.eu/

Trac Browser - https://themes.trac.wordpress.org/browser/reykjavik/0.9.5

SVN - https://themes.svn.wordpress.org/reykjavik/0.9.5
ZIP - https://wordpress.org/themes/download/reykjavik.0.9.5.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=reykjavik/0.9.4&new_path=reykjavik/0.9.5

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.11 reviewing

(this ticket)


https://themes.svn.wordpress.org/reykjavik/0.9.5/screenshot.jpg

Change History (43)

#2 @poena
2 weeks ago

  • Owner set to poena
  • Status changed from new to reviewing

#3 @poena
2 weeks ago

Hi
This is a partial review only, because of it's size and complexity. I have not yet looked through the entire includes/plugin folder.
I read the past review again and was not sure if @kevinhaig wanted to continue.

The theme is very well coded overall, and I liked how you used the starter content, but there are also some things that are plugin territory.

When I press the button that says "Install theme demo content" I get the following message:
"Sorry, you are not allowed to access this page."
-I can't test the theme or review the demo content if it is not complete.


Second opinion needed:
ping @greenshady @thinkupthemes (you have both looked at the theme before)
CSS generation

Unregistering the two default widgets. While I think the custom text widget could be allowed as minor content creation, the defalt widgets should not be removed.

add_filter( 'widget_text', 'do_shortcode' );
See https://core.trac.wordpress.org/changeset/41361

Since you have included documentation in the theme folder, it needs to be reviewed as part of the theme.
We don't allow use of cdn; Include all scripts and resources it uses rather than hotlinking. The exception to this is Google Fonts.
The problem with hot linking images is that these can be changed at any time, which means we can't review them properly so they need to be included as well.


Required

Plugin territory:
Adding buttons or making other changes to functionality of the tiny mce editor. -Themes are meant for presenting content. As well as these options work for your specific theme, themes are not for adding ways to style the content. This will be inconsistent since when the user switches themes these option will not be there. If these options were in a plugin, they would remain even while switching themes.

Child theme creation (class-use-child-theme.php)

--
Why is the code for the default logo still in site-branding.php?

The starter content is meant to be edited, but the user should not need to remove your social links or the link to the documentation, which are used in the starter content.

esc_attr() is escaping for HTML attributes. esc_html() is escaping for HTML blocks.
See https://vip.wordpress.com/documentation/validating-sanitizing-escaping/#escaping-securing-output
breadcrumps.php line 38 replace esc_attr_e() with esc_html_e().
class-loop.php line 130 replace esc_attr__() with esc_html__().

Use the version of TGMPA that is specific for themes redistributed on WordPress.org.
http://tgmpluginactivation.com/download/

In get_image_id_from_url() do not use guid as an url, it is a unique identifier and it is not updated if for example the site is moved.
See: https://codex.wordpress.org/Changing_The_Site_URL#Important_GUID_Note
Perhaps this can help? https://developer.wordpress.org/reference/functions/attachment_url_to_postid/

In link_skip_to() the text seem to be escaped twice?


Accessibility

https://make.wordpress.org/themes/handbook/review/accessibility/required/#headings

Keyboard Navigation -did not pass
Mostly good, but after I tab through the responsive version of the menu and I close the menu by pressing enter when I reach the close button, I expect the focus to return to the main page.
Instead, the next focusable item is the first menu item again, eventhough I can no longer see the menu.

Controlls -pass
Skip Links -pass
Forms -pass
Headings -did not pass
The custom widget for recent posts does not have a h3 widet title by default, so the headings skip a level, h2-h4.

ARIA Landmark Roles -pass
Link Text -pass
Contrast -pass

Images -did not pass
Icons and icon fonts: If the icon is supplementing text then the icon should be hidden from screen readers using aria-hidden. For example the smaller icons used in the widget list.

Media -Starter content specific -did not pass
Media resources must NOT auto start or change without user action as a default configuration.
This includes resources such as audio, video, or image/content sliders and carousels.

Screen Reader Text -pass
Not Allowed -none found

Last edited 2 weeks ago by poena (previous) (diff)

#4 @webmandesign
2 weeks ago

Hi @poena,

Thank you for your review! Here is just my quick reply to make sure I understood correctly before making the changes:


From "Second opinion needed" section:

@kevinhaig wanted to continue?

As far as I'm concerned @kevinhaig approved the theme with this input:

If a plug in is used then it becomes a 2 step process. You can't accidentally or curiously click a button and whamo content is added. There is a purposeful intent with a plugin.

"Install theme demo content"

First, sorry, I haven't hidden this properly. This will be ready after I finish the theme demo website, which is obviously tied to this theme review too. Please read my previous reply (point 7, and this reply too) for more info.

Unregistering the two default widgets

To explain, I'm using the same functionality from the widgets, just enhancing them with some custom improvements. Should I rather create a new widgets instead?

add_filter( 'widget_text', 'do_shortcode' );

Thanks for pointing this out. I know about the new Text widget shortcodes treatment, however, I've kept this code in for backwards compatibility. Not everyone is using the most recent WordPress version, especially in different language than English. Does this code create some issues, should I remove it?

Documentation in theme folder

I actually keep images hot linked exactly because I might to edit them in the future. This happened to me previously, either because the WordPress interface changed or because I improved the procedure described via the image. In this case I've rather decided to remove the documentation from the theme folder.


From "Required" section:

Plugin territory

I actually think this is very much a theme territory. The thing is that theme provides classes and styles for HTML elements, which are presentational. I'm just extending TinyMCE native "Formats" dropdown button to include these theme styles (and adding missing TinyMCE HTML tags) so users are able to use them easily. Otherwise they would need to switch to "Text" tab of the editor and apply them using HTML code, which is not very friendly and still would not resolve the switching themes issue.

Providing these as a plugin is again not very productive as the styles are coming from the theme itself. And implementing styles without any helpful user interface is just wasting time I think as nobody would ever use them. With "Formats" dropdown there is more chance of using the styles, which will even help with creating more accessible post/page content. So, should I really remove this?

Also, this is kind of funny, as I always read ridiculous articles about using shortcodes for a simple HTML stylings. But now if I can't replace it with easy to use "Formats" dropdown in TinyMCE, what should I do? :) HTML is not easy for basic WordPress users. Even those shortcode would be easier for many of users from my experience ;) (Note that I'm not promoting using shortcodes for these styles! I always wanted to use "Formats" dropdown for them and not just bury them in theme documentation "how to use custom HTML"...)

Child theme creation

This class was actually created for use in themes and @greenshady and @emiluzelac were involved in it as far as I remember. Is this not allowed in WPORG themes?

Why is the code for the default logo still in site-branding.php?

I still keep it in my themes as I use it in my theme demo website server. User will not actually get to run the code ever. Should I remove it still?

The starter content social and documentation links

Well, I think users will nonetheless need to edit the starter content. Showcasing social icons is the easiest way to let them know about their existence. If I can't use my social network links, can I use some WordPress ones? And providing link to online documentation will help new users start with the theme and lower the amount of potential support questions.

TGMPA version

I'm not sure what you mean here. I am using the version 2.6.1. I have followed your link and generated the TGMPA again, and it's exactly the same one I am using. Could you please provide more info about this issue?


From "Accessibility" section:

Headings - Recent Posts widget title

Well, like I've said previously, I'm just enhancing the widgets and not modifying their code where not needed. I'm using the default Recent Posts widget behavior here and when there is no title set for the widget, it simply doesn't display the title. This should probably be also fixed in WordPress core then. Or have you found any other headings issues?

Images - Icons font

You probably mean the icons such as the bubble in Recent Comments widget, for example. Am I correct? These icons are decorative images added using CSS only and they have speak: none; style applied. Or do you mean some other icons I'm not aware of?

Media - Starter content custom header video

So, does this mean I should just remove the custom header video from starter content, or should I make the whole custom header video functionality not to start automatically (I'm just using WordPress native functionality there)?


Thanks in advance for your explanations!

Best regards,

Oliver

Last edited 2 weeks ago by webmandesign (previous) (diff)

#5 @poena
2 weeks ago

Hi

-No as far as I know neither creating child themes -even with user consent -nor adding this kind of functionality to the tiny mce editor is allowed.

Should I rather create a new widgets instead?
-Yes

-Up until this change, it is up to the individual plugin authors to decide if they meant for their shortcodes to work in widgets.

-The social icons will work fine if you use the base url for example https://www.facebook.com/ instead of advertising your own social links.

-Yes, remove any and all code that is not used by this version of the theme.

I did not see the speak:none for the icons, -that should be fine.

The accessibility requirements do require media not to auto start,
https://make.wordpress.org/themes/handbook/review/accessibility/required/#media
But after some digging trying to find out why this wasn't an existing setting, I also found this comment:
Autoplay is okay for decorative videos.
https://core.trac.wordpress.org/ticket/38172#comment:13
So yeah that should be fine too.

-But the default recent post widget does display the widget title,
and the theme does skip this heading level and it needs to be fixed.

#6 @webmandesign
2 weeks ago

Hi,

TinyMCE Formats dropdown

Sorry, I still don't get the Formats dropdown issue in TinyMCE. This is HTML styles related, so why it is considered a plugin territory?

You see, on one hand the theme is all about presentation. So I create some pretty styles users can use. These are all just basic HTML with custom CSS class(es) applied to them. But I can not implement an easy interface to use them as it is not allowed by the themes? That seems to me contra-productive. Sure users can edit HTML directly, but is this really a good user experience?

Creating a plugin for these few custom styles seems to be too much of an overhead while still the styles may not be portable to another theme...

Can someone else provide opinion here please?

Child themes creator

Seems to me like we can use this script in themes. @greenshady and @emiluzelac, can you please provide input on this?

TGMPA version

Could you please provide explanation for that issue so I can act appropriately?

Thanks!

Oliver

#7 @webmandesign
2 weeks ago

TinyMCE Formats dropdown

I've also found some more information about "Formats" dropdown in WordPress codex. It actually says these custom styles should be added via the theme:

Whenever possible, style related code should be in the theme in which the styles are implemented. The most natural location is the functions.php file, though of course the filters could also be added in a plugin if desired.

So, is this a plugin territory or can I leave it in theme?

#8 follow-up: @webmandesign
2 weeks ago

Custom Text and Recent Posts widgets enhancements

Actually, I'd like to discuss this more too ;)

If I create additional 2 widgets within the theme instead of reusing and enhancing the WordPress native ones, user might end up with unrecognized (and removed) widgets if he/she decides to switch the theme.

The enhancements I apply are actually forwards and backwards compatible, meaning once user switches the theme, they fall back to default Text and Recent Posts widget display.

So, is it really required to add 2 new widgets by the theme or can I keep the enhancements the way they are?

#9 @jrf
2 weeks ago

Regarding TGMPA: If you download a fresh copy of TGMPA using the Custom TGMPA Generator and choose "wordpress.org" as distribution channel, it will serve you with a customized download of TGMPA which will pass the Theme Check rules (text-domain and such).

#10 @webmandesign
2 weeks ago

Hi @jrf,

Thanks for explanation, but that's exactly what I did. I just added a theme PHP file comment header to the file. Otherwise it's identical with that generated version. And that's why I'm asking what is wrong with it ;)

Regards,

Oliver

Last edited 2 weeks ago by webmandesign (previous) (diff)

#11 in reply to: ↑ 8 @poena
2 weeks ago

Since nearly everyone is pinged in the ticket, I'm sure you will receive second opinions soon.

I still believe strongly that themes should not make changes to the admin including the editor.
While this is more important when users try to add different icon selectors and similar to the editor, I don't agree that this is theme specific. Consistency is important not only for new users.
We as theme authors still have the ability to use the editor style and to use different post and page templates.
-The codex is is not limited by the guidelines for themes submitted to WordPress.org, the documentation is for themes in general.

#12 @webmandesign
2 weeks ago

I am also using editor style in the theme and also different post and page templates. But these Formats dropdown styles has nothing to do with post and page templates. They can be used anywhere in content and thus are template independent.

Here is an example post with all the custom TinyMCE Formats dropdown styles the theme adds for better reference: https://themedemos.webmandesign.eu/reykjavik/post-3/

As you can see, some of those are pretty theme specific styles and making them portable to other themes with a plugin might cause layout issues in those themes. This way the HTML is kept in the post/page content when user applies the style from Formats dropdown and when he/she switches the theme, it is a matter of simple custom CSS to reintroduce the missing styles again.

#13 @jrf
2 weeks ago

Thanks for explanation, but that's exactly what I did. I just added a theme PHP file comment header to the file. Otherwise it's identical with that generated version. And that's why I'm asking what is wrong with it ;)

@webmandesign It's fine to add an extra theme file header, but in that case, leave the TGMPA file header intact.
There are a lot of (incorrectly manually edited) versions of TGMPA floating out there. The Custom Generator was created to solve the problems we saw end-users having with that (white screens of death and such).

Unless someone does a full file compare, the customized file header is the only quick way to check if someone is using the correct TGMPA version.
And considering theme reviewers have enough to do as it is, please allow them to use the quick way to verify that you are using the correct version by leaving the TGMPA headers (including the one generated by the Custom Generator) in place.

#14 @webmandesign
2 weeks ago

Hi @jrf,

I completely understand. I keep the original file header not only for theme reviewer, but for my future reference too! :) Please see the file at https://github.com/webmandesign/reykjavik/blob/master/library/includes/vendor/tgmpa/class-tgm-plugin-activation.php

The only difference from the TGMPA generator header is that I have removed the for parent theme Reykjavik for publication on WordPress.org portion from the @version 2.6.1 line. That is pretty insignificant change as you can tell.

Last edited 2 weeks ago by webmandesign (previous) (diff)

#15 @jrf
2 weeks ago

@webmandesign Actually, no, that's not an insignificant change as that's the part through which theme reviewers can do a quick check. If you remove that, they would have to do a full file compare or will presume you have used the generic version.

#16 @webmandesign
2 weeks ago

@jrf Thanks for surprising explanation! I'll go ahead and add the comment back to the file.

#17 @themetracbot
13 days ago

  • Summary changed from THEME: Reykjavik – 0.9.5 to THEME: Reykjavik – 0.9.6

Reykjavik - 0.9.6

Reykjavik is fresh, lightweight, speed and SEO optimized, accessibility ready WordPress theme perfect for your next business, portfolio, blog or WooCommerce e-shop website. You can customize all elements of the theme to your needs. The theme works perfectly with Beaver Builder, Beaver Themer, Elementor, Visual Composer and other page builders to create fantastic layouts. It features mobile-optimized codebase and design with unique, easy-to-access mobile navigation. Build your website in no time with integrated one-click demo import functionality. Impress your website visitors with this beautiful theme now!

Theme URL - https://www.webmandesign.eu/portfolio/reykjavik-wordpress-theme/
Author URL - https://www.webmandesign.eu/

Trac Browser - https://themes.trac.wordpress.org/browser/reykjavik/0.9.6

SVN - https://themes.svn.wordpress.org/reykjavik/0.9.6
ZIP - https://wordpress.org/themes/download/reykjavik.0.9.6.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=reykjavik/0.9.5&new_path=reykjavik/0.9.6

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.11 reviewing

(this ticket)


https://themes.svn.wordpress.org/reykjavik/0.9.6/screenshot.jpg
Theme Check Results:

  • Warning: More than one text-domain is being used in this theme. This means the theme will not be compatible with WordPress.org language packs. The domains found are reykjavik, {%= text_domain %}

#18 @themetracbot
13 days ago

  • Summary changed from THEME: Reykjavik – 0.9.6 to THEME: Reykjavik – 0.9.7

Reykjavik - 0.9.7

Reykjavik is fresh, lightweight, speed and SEO optimized, accessibility ready WordPress theme perfect for your next business, portfolio, blog or WooCommerce e-shop website. You can customize all elements of the theme to your needs. The theme works perfectly with Beaver Builder, Beaver Themer, Elementor, Visual Composer and other page builders to create fantastic layouts. It features mobile-optimized codebase and design with unique, easy-to-access mobile navigation. Build your website in no time with integrated one-click demo import functionality. Impress your website visitors with this beautiful theme now!

Theme URL - https://www.webmandesign.eu/portfolio/reykjavik-wordpress-theme/
Author URL - https://www.webmandesign.eu/

Trac Browser - https://themes.trac.wordpress.org/browser/reykjavik/0.9.7

SVN - https://themes.svn.wordpress.org/reykjavik/0.9.7
ZIP - https://wordpress.org/themes/download/reykjavik.0.9.7.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=reykjavik/0.9.6&new_path=reykjavik/0.9.7

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.11 reviewing

(this ticket)


https://themes.svn.wordpress.org/reykjavik/0.9.7/screenshot.jpg

#19 @rabmalin
12 days ago

  • Buttons in admin editor seems clearly plugin territory. Theme is for displaying what the data system has. Adding extra markup in user content is not theme's job I believe.
  • Use_Child_Theme - This is also plugin territory. Reviewer here in TRT are also active in the WordPress community outside TRT. But this does not necessarily mean those are in line with the Theme Review guideline.
  • CSS compiling/minification is also plugin territory. There are several plugins out there for CSS minification and related.

#20 @webmandesign
12 days ago

Hi @rabmalin,

Thank you for the input.

Editor "Formats" dropdown

Well, than this is pretty confusing for theme developers. There is the information in WP codex first (see my [#comment:7 previous reply). But now I'm told this is plugin territory.

Well, what's the point for a plugin enabling "Formats" dropdown with some small additional HTML tags (such as <mark> or <small>) while keeping the other custom styles for the theme to set up via the plugin? Because for me creating a plugin with the theme's custom styles is quite nonsense as other themes do not support the same custom style CSS classes.

And yet, custom widgets are allowed in WPORG themes, while custom "Formats" dropdown is not. I strongly think the "Formats" styles are more of a theme territory than custom widgets.

Just for anybody interested, please, have a look at example of what "Formats" custom styles I'm talking about at https://themedemos.webmandesign.eu/reykjavik/post-3/

It seems like I'm left with explaining theme users how to code HTML in this case...

Child theme generator

Very unfortunate, but seems I can not do anything here...

Generated CSS stylesheet

I am generating a single theme stylesheet using WordPress Filesystem API in wp-content/uploads/wmtheme-THEMESLUG folder only because this combines the default theme stylesheets and user's Customizer settings styles. Surely, I minimize the stylesheet as this is a good optimization practice. But I also keep the unminified version of generated stylesheet (with dev- prefix).

So, does this really mean WPORG themes are all destined to output Customizer settings styles in HTML head? I find that very ugly, unprofessional and shortcut solution when we can do better. Besides with generated stylesheet I can easily apply these Customizer styles for editor stylesheet too.

Regards,

Oliver

Last edited 12 days ago by webmandesign (previous) (diff)

#21 @webmandesign
12 days ago

Hi,

Here is my understanding of the issues here:


I'm advised this:

"Formats" dropdown issue
Use plugin => User need to install a plugin. When user switches the theme, custom unstyled HTML will be kept in place. Which is a perfect fallback.

Custom widgets enhancements issue
Create new widgets instead => When user switches the theme, all the custom widgets are lost.

Generating CSS stylesheet issue
Do not use => Output Customizer styles into HTML head. I can still use SASS minified other theme stylesheets, though.

Child theme generator issue
Do not use => User needs to create its own child theme. Or download a sample one.


In opposite to current theme implementation:

"Formats" dropdown issue
When user switches the theme, custom unstyled HTML with be kept in place. Which is a perfect fallback.

Custom widgets enhancements
When user switches the theme, the custom enhanced widgets fall back to default widgets functionality gracefully. Widgets and their contents are kept.

Generating CSS stylesheet issue
Customizer styles and default theme styles are kept in a single, optimized stylesheet file generated using WordPress Filesystem API in wp-content/uploads folder. Unminified version is generated too. No clutter in HTML head. Customizer styles are reused in editor stylesheet too for the best user experience.

Child theme generator issue
User just activates a pre-set child theme when he/she decides to do some custom theme modifications.
(I understand this may not be supported in TRT, but actually, I still think there was some intention why @greenshady and @emiluzelac were involved in the project. If it was intended for a plugin, there were similar plugin solutions in WPORG repo already, as far as I know.)


Could more TRT members provide feedback on this? I have just 2 opinions for now.

Thanks and regards,

Oliver

Last edited 12 days ago by webmandesign (previous) (diff)

This ticket was mentioned in Slack in #themereview by webmandesign. View the logs.


12 days ago

#23 @greenshady
12 days ago

In the past, we were pretty consistent about allowing themes to hook into the editor to add custom formatting options. This was the proposed solution we gave to theme authors who wanted to use shortcodes for this sort of thing because it was forward compatible. This is something the team agreed to several years ago. We have not changed since then. As long as the theme is using basic HTML and classes, it shouldn't be an issue.

On the subject of a child theme creator, that should be handled by a plugin.

For generating stylesheets, you should add any dynamic styles to wp_head. Or, use wp_add_inline_style() if it depends on another style.

#24 @williampatton
12 days ago

Hi,

  • Formats drowpdown - I too believe a feature such as this is best suited in a plugin so that it could be used across many themes with ease. I would encourage you to create a plugin for this in future as it may be useful for yourself and several other people.
    • That being said I do not think that in this specific case it makes sense to strip out the feature and make it into a plugin as the current implementation gives a much better experience for users. It also falls back gracefully should a theme be changed. One possible improvement might be to prefix the classnames so that they are even more likely not to be styled in future themes.
  • Custom widgets - Extending widgets is my preference here, it is better for the user to keep the content of a text widget minus the custom theme additions than it is for them to loose everything on switch. I would say that you need to be careful when extending classes as to not override anything that may break core expected compatibility.

#25 @webmandesign
10 days ago

Hi @greenshady and @williampatton,

Thank you guys for your feedback on this.
Here is a short summary if I understood correctly (but not everybody voiced for each issue and I'm obviously excluding my voice too ;) ):


"Formats" dropdown issue
2 voices against, 2 voices for.

I understand the point of implementing this into a plugin. But that would work only for some global custom styles such as font sizes, dropcap paragraph, uppercase text, text columns, font weights and possibly pullqoutes and link buttons.

But Reykjavik theme adds styles that are quite specific (and maybe specific also to my other themes), such as outdented content, font family classes and heading classes. These would need to be added to the plugin hooking onto a filter through the theme, which would still produce the same issue of non-portability if the theme is switched (everything would still gracefully fall back though).

Other solution would be to instruct user on how to add these HTML classes and HTML elements via "Text" tab of editor in theme documentation. But that still produces the portability issue.

As for unique class names, I'd rather keep those more generic if possible. Generic classes would provide more compatibility when switching themes, I'd say. It's better if a theme contains a style for .weight-300 class than .reykjavik-weight-300. Prefixing the class would also mean I need to add these prefixed classes into all of my other compatible-styles themes, and future themes classes too, which would be manageability nightmare.


Custom widgets enhancements
2 voices for new custom widgets, 1 voice for enhancements of WP native widgets.

Yes, I am as careful as possible with extending widget classes. The biggest problem nowadays is with Text widget as it gets updated with each WordPress version, but I keep track of that and update my enhancements as needed. (It would be easier if there are more hooks in the widget I could use...)


Generating CSS stylesheet issue
Seems everybody is against.

I would like to ask here why exactly, what is the objection here?

The functionality in the theme is coded using WordPress APIs and functions, nothing super magical here, and passes both theme check plugins checks.

I know I could output the styles into HTML head as everybody else does, it would be easier, but if I can do better, why not use that?


Child theme generator issue
Everybody against.

Like I've said, I understand this may be a bit too much for a theme to provide, though it simplifies the process for end user. Sadly, I will remove this functionality from Reykjavik.


Thanks and regards,

Oliver

Last edited 10 days ago by webmandesign (previous) (diff)

#26 @greenshady
10 days ago

Formats drop-down issue

This is allowed under our current guidelines. Not only that, it's a practice that we've encouraged. The notion that it needs to moved into a plugin is merely something that's been brought up in this ticket and does not reflect the team's historical stance on this. In essence, it is allowed in your theme.

Widgets issue

Looks like the theme is handling this correctly and in a forward-compatible manner.

The guideline is that if you unregister any of the default WP widgets, you must replace them with widgets that perform at least the same job. We also said that you're not allowed to unregister the text widget (because text widgets are considered content). However, we should be able to make an exception if it is forward-compatible.

We used to have all this written down somewhere. Here it is in partiality (missing some of the important bits I said above): https://make.wordpress.org/themes/handbook/review/recommended/#code

Ever since we changed over to the handbook a few years back, the guidelines have been less clear.

Generating a stylesheet

We've just been over this ad nauseam in the past, so this is an old discussion for most of us. But, basically, read this: http://ottopress.com/2011/tutorial-using-the-wp_filesystem/

There's plenty of details about why you should not do it.

#27 @webmandesign
10 days ago

Thank you for details @greenshady!

@poena is it OK with you if I remove the child theme generator and make the custom CSS output to HTML head in my next theme update, and just leave "Formats" dropdown and widgets enhancements the way they are currently implemented, please?

Thanks!

Oliver

#28 @poena
10 days ago

Hi
I am bit sick so I will catch up as soon as I can! :)

#29 @webmandesign
10 days ago

@poena Sure thing! Take a rest and wish you get fit fast :)

#30 @webmandesign
10 days ago

Hi @greenshady,

I've read the Otto's article you've provided link to. Actually, I've read it in the past already and it basically confirmed me that I can go ahead and use the WP_Filesystem API to generate the CSS. That's why I coded this functionality in my themes framework.

This time I went through the article comments too. I can't really find anything pointing out I'm doing something very bad.

Everything seems to be about security. And I do care about security very much, and if I understand correctly from the article, while I use WP_Filesystem API and I escape the CSS being written to the file (which I do), there should be no issue with my implementation.

Or am I missing something? I feel a bit frustrated now, also about this issue as it seems like I either don't understand or I'm not explaining things properly. Maybe, if you are interested and find some time, please have a look at `reykjavik/library/includes/classes/class-customize-styles.php` file. IT's still pretty mysterious for me why I have to output CSS to HTML head instead - the article hasn't really provided the answer.

But, to push things forward with the theme review, I'm going to go with CSS in HTML head for now, unfortunately...

Thanks and regards,

Oliver

#31 @webmandesign
9 days ago

Hi guys,

I know this is not really a place to ask this type of question, but as I can not generate CSS files any more, I've run into dead end regarding editor custom styles. Specifically, there seems to be no way for me to apply customizer custom styles onto TinyMCE editor content.

I've already tried hooking onto TinyMCE $init and add inline styles the same way I'm advised to do on theme front-end. Using $init['content_style'] setting allows you to do this. However, it does not work very well:

  • it is being outputted before the actual editor stylesheet(s) are loaded, which mean I would have to rise the CSS specificity, and so effectively duplicating custom styles code creating separate one for editor and for website front-end,
  • it allows for 10000 character CSS string only (which I just exceed).

Do you know (maybe @greenshady ?) a workaround for this?
Have anybody tried to apply custom styles generated by Customizer on TinyMCE editor before?
Or is it just me trying this really?

Note that this worked perfectly fine for me when I could generate CSS files. It produced expected behavior for users: when, for example, content area colors are changed in customizer, these colors apply onto TinyMCE editor content too.

Thanks for any input!

Regards,

Oliver

#32 @themetracbot
7 days ago

  • Summary changed from THEME: Reykjavik – 0.9.7 to THEME: Reykjavik – 0.9.8

Reykjavik - 0.9.8

Reykjavik is fresh, lightweight, speed and SEO optimized, accessibility ready WordPress theme perfect for your next business, portfolio, blog or WooCommerce e-shop website. You can customize all elements of the theme to your needs. The theme works perfectly with Beaver Builder, Beaver Themer, Elementor, Visual Composer and other page builders to create fantastic layouts. It features mobile-optimized codebase and design with unique, easy-to-access mobile navigation. Build your website in no time with integrated one-click demo import functionality. Impress your website visitors with this beautiful theme now!

Theme URL - https://www.webmandesign.eu/portfolio/reykjavik-wordpress-theme/
Author URL - https://www.webmandesign.eu/

Trac Browser - https://themes.trac.wordpress.org/browser/reykjavik/0.9.8

SVN - https://themes.svn.wordpress.org/reykjavik/0.9.8
ZIP - https://wordpress.org/themes/download/reykjavik.0.9.8.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=reykjavik/0.9.7&new_path=reykjavik/0.9.8

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.11 reviewing

(this ticket)


https://themes.svn.wordpress.org/reykjavik/0.9.8/screenshot.jpg

#33 @webmandesign
7 days ago

Hi,

The theme update 0.9.8 contains these changes:

  • Disabled child theme generator functionality but kept it in the code as it is a part of my theme framework,
  • Switched from using stylesheet generator to outputting custom styles into HTML head (unfortunately, this can not be applied on TinyMCE edito styles - see my previous replies),
  • Demo content import functionality was enabled as the theme demo files are ready now,
  • I've kept the functionality of "Formats" dropdown and custom widgets enhancements untouched,
  • Responsive and other styles were fixed and updated.

Thanks and regards,

Oliver

#34 @greenshady
6 days ago

Here's a look at how I handle custom colors and the visual editor:
https://github.com/justintadlock/stargazer/blob/master/inc/custom-colors.php

In particular, here's the action hook call:
https://github.com/justintadlock/stargazer/blob/master/inc/custom-colors.php#L56-L58

And, the callback:
https://github.com/justintadlock/stargazer/blob/master/inc/custom-colors.php#L128-L135

Edit: I almost forgot the most important bit.

You also need to add this to your add_editor_style() call for your theme:
https://github.com/justintadlock/stargazer/blob/master/inc/stargazer.php#L277

That's going to generate the Ajax hook needed for the other parts of the code.

Last edited 6 days ago by greenshady (previous) (diff)

#35 @webmandesign
6 days ago

Hi @greenshady,

Thank you for the solution. I was actually using similar kind of solution in the past and am not really a fan of it. But I guess that would be the only solution here for me.

Though, I don't understand why this is OK with TRT and generating stylesheets securely the way Otto described is not.

I'll go forward and update the theme with your code. Thanks again!

Regards,

Oliver

#36 @webmandesign
6 days ago

@greenshady BTW, very clever usage of AJAX here without a need to create a dedicated "CSS PHP file"! Thanks! I've learned something new :)

Last edited 6 days ago by webmandesign (previous) (diff)

#37 @themetracbot
5 days ago

  • Summary changed from THEME: Reykjavik – 0.9.8 to THEME: Reykjavik – 0.9.9

Reykjavik - 0.9.9

Reykjavik is fresh, lightweight, speed and SEO optimized, accessibility ready WordPress theme perfect for your next business, portfolio, blog or WooCommerce e-shop website. You can customize all elements of the theme to your needs. The theme works perfectly with Beaver Builder, Beaver Themer, Elementor, Visual Composer or other page builder to create fantastic layouts. It features mobile-optimized codebase and design with unique, easy-to-access mobile navigation. Build your website in no time with integrated one-click demo import functionality. Impress your website visitors with this beautiful free theme!

Theme URL - https://www.webmandesign.eu/portfolio/reykjavik-wordpress-theme/
Author URL - https://www.webmandesign.eu/

Trac Browser - https://themes.trac.wordpress.org/browser/reykjavik/0.9.9

SVN - https://themes.svn.wordpress.org/reykjavik/0.9.9
ZIP - https://wordpress.org/themes/download/reykjavik.0.9.9.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=reykjavik/0.9.8&new_path=reykjavik/0.9.9

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.11 reviewing

(this ticket)


https://themes.svn.wordpress.org/reykjavik/0.9.9/screenshot.jpg

#38 @webmandesign
5 days ago

Hi @poena,

Here is what has changed in theme update 0.9.9:

  • Theme screenshot updated
  • Making custom styles work in Visual Editor (TinyMCE) (thanks to @greenshady)
  • Updated theme starter content with new WooCommerce 3.2+ shortcodes
  • Fixed and updated custom styles functionality
  • Updated styles
  • Fixed WooCommerce 3.2+ issue when page template is assigned for a product
  • Improving page templates
  • Adding page templates also for Jetpack Portfolio

Thanks and regards,

Oliver

#39 @themetracbot
3 days ago

  • Summary changed from THEME: Reykjavik – 0.9.9 to THEME: Reykjavik – 0.9.10

Reykjavik - 0.9.10

Reykjavik is fresh, lightweight, speed and SEO optimized, accessibility ready WordPress theme perfect for your next business, portfolio, blog or WooCommerce e-shop website. You can customize all elements of the theme to your needs. The theme works perfectly with Beaver Builder, Beaver Themer, Elementor, Visual Composer or other page builder to create fantastic layouts. It features mobile-optimized codebase and design with unique, easy-to-access mobile navigation. Build your website in no time with integrated one-click demo import functionality. Impress your website visitors with this beautiful free theme!

Theme URL - https://www.webmandesign.eu/portfolio/reykjavik-wordpress-theme/
Author URL - https://www.webmandesign.eu/

Trac Browser - https://themes.trac.wordpress.org/browser/reykjavik/0.9.10

SVN - https://themes.svn.wordpress.org/reykjavik/0.9.10
ZIP - https://wordpress.org/themes/download/reykjavik.0.9.10.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=reykjavik/0.9.9&new_path=reykjavik/0.9.10

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.11 reviewing

(this ticket)


https://themes.svn.wordpress.org/reykjavik/0.9.10/screenshot.jpg

#40 @themetracbot
3 days ago

  • Summary changed from THEME: Reykjavik – 0.9.10 to THEME: Reykjavik – 0.9.11

Reykjavik - 0.9.11

Reykjavik is fresh, lightweight, speed and SEO optimized, accessibility ready WordPress theme perfect for your next business, portfolio, blog or WooCommerce e-shop website. You can customize all elements of the theme to your needs. The theme works perfectly with Beaver Builder, Beaver Themer, Elementor, Visual Composer or other page builder to create fantastic layouts. It features mobile-optimized codebase and design with unique, easy-to-access mobile navigation. Build your website in no time with integrated one-click demo import functionality. Impress your website visitors with this beautiful free theme!

Theme URL - https://www.webmandesign.eu/portfolio/reykjavik-wordpress-theme/
Author URL - https://www.webmandesign.eu/

Trac Browser - https://themes.trac.wordpress.org/browser/reykjavik/0.9.11

SVN - https://themes.svn.wordpress.org/reykjavik/0.9.11
ZIP - https://wordpress.org/themes/download/reykjavik.0.9.11.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=reykjavik/0.9.10&new_path=reykjavik/0.9.11

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.11 reviewing

(this ticket)


https://themes.svn.wordpress.org/reykjavik/0.9.11/screenshot.jpg

#41 @webmandesign
3 days ago

Hi @poena,

Here is what has changed in 0.9.10 and 0.9.11 theme udpate:

  • Updating Text widget to WordPress 4.9 code. This, however, produces "Overriding WordPress globals is prohibited" error generated in NS Theme Check plugin test. Is this OK with you or do you have any suggestions what I could do?
  • Adding WebMan Templates plugin into recommended plugins list.
  • Updating framework (library folder, changelog file within).

Please note that the theme is ready for 1.0.0 release version from my point of view now as I have no more code TODOs in the list.

Thanks and regards,

Oliver

#42 @poena
2 days ago

  • Owner poena deleted

I am not able to continue the review. I have tried to find a moderator to take over since thursday, but since no one has said yes I have no option but returning it to the queue.

#43 @webmandesign
2 days ago

OK, no worries, thank you for update and for the review provided @poena

Note: See TracTickets for help on using tickets.