WordPress.org

Make WordPress Themes

Opened 3 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#46546 closed theme (live)

THEME: Reykjavik – 0.9.14

Reported by: webmandesign Owned by: acosmin
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.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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

Change History (75)

#2 @poena
3 months ago

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

#3 @poena
2 months 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 months ago by poena (previous) (diff)

#4 @webmandesign
2 months 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 months ago by webmandesign (previous) (diff)

#5 @poena
2 months 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 months 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 months 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 months 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 months 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 months 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 months ago by webmandesign (previous) (diff)

#11 in reply to: ↑ 8 @poena
2 months 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 months 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 months 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 months 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 months ago by webmandesign (previous) (diff)

#15 @jrf
2 months 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 months ago

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

#17 @themetracbot
2 months 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.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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
2 months 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.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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

#19 @rabmalin
2 months 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
2 months 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 2 months ago by webmandesign (previous) (diff)

#21 @webmandesign
2 months 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 2 months ago by webmandesign (previous) (diff)

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


2 months ago

#23 @greenshady
2 months 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
2 months 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
2 months 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 2 months ago by webmandesign (previous) (diff)

#26 @greenshady
2 months 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
2 months 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
2 months ago

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

#29 @webmandesign
2 months ago

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

#30 @webmandesign
2 months 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
2 months 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
2 months 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.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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

#33 @webmandesign
2 months 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
2 months 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 2 months ago by greenshady (previous) (diff)

#35 @webmandesign
2 months 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
2 months 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 2 months ago by webmandesign (previous) (diff)

#37 @themetracbot
2 months 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.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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

#38 @webmandesign
2 months 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
2 months 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.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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

#40 @themetracbot
2 months 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.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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

#41 @webmandesign
2 months 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 months 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 months ago

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

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


2 months ago

#45 @poena
2 months ago

  • Owner set to acosmin

#46 @acosmin
2 months ago

Hi @webmandesign


  1. includes\tgmpa\class-tgmpa-plugins.php
    • L127, you're allowed to only recommend plugins hosted on .org. You can add a link to that plugin in a readme file or your documentation on your website.
  1. \templates\parts\footer\site-info.php
    • L48, use date_i18n( 'Y' )
  1. Remove readme.txt if you're not using it.
  1. You'll need to provide license information for images located in
    • assets\images\starter-content, assets\images\svg
  1. Theme URL doesn't work.
  1. accessibility-ready, You'll need an accessibility review, it might take some time to find a reviewer.

#47 @webmandesign
2 months ago

Hi @acosmin,

  1. Oh, sorry about this. I must have got carried away ;) Will fix in upcoming update.
  2. Thanks for this! Will implement.
  3. OK, sounds good to me :)
  4. I'll do.
  5. I'm waiting for the theme to be made live and then I will publish the theme description page on my website. There is no point to have that page live for 3 month already while the theme is not released ;) Anyway, the page will look similarly to https://www.webmandesign.eu/portfolio/icelander-wordpress-theme/ if that helps.
  6. I thought @poena already provided the accessibility review above and I've fixed the issues reported. Or wasn't this the "real accessibility review" yet?

Thanks for update!

Regards,

Oliver

#48 @acosmin
2 months ago

:) @poena is the theme ready from an accessibility point of view?

#49 @poena
8 weeks ago

If I remember correctly it needs a follow up of the items that it did not pass,
and a review of the changes made to the editor.

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.

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

But I think the regular a11y reviewers are back now. There is a bit of a back log still.


5) -the link is optional, so you can remove it and then add it when the page is ready.

#50 @acosmin
8 weeks ago

@poena The editor modifications are just styles to be applied on content. It seems to be done correctly.

As I know, we don't have any rules against that, some would argue that adding styles is what a theme should do :)

What would be the next step, marking it as approved or keeping it open until an a11y reviewer does his review?

#51 @poena
8 weeks ago

Has the formats option and buttons in the editor been removed? I haven't reviewed the latest versions.

Once you and @greenshady and @joedolson feel it is ready it should be set live, not approved. The theme has already waited quiet long.

#52 @webmandesign
8 weeks ago

Hi @poena,

No, Formats dropdown functionality is still kept in the theme. Please see the whole conversation above, around #comment:26

I will upload a new theme version fixing @acosmin reported issues today.

Regards,

Oliver

#53 @webmandesign
8 weeks ago

Actually, sorry, I was too quick in my plan of theme update upload ;) The update fixing the issues is ready, but I'm waiting for WebMan Templates plugin to be checked and added to WPORG plugins repo. So I will upload the theme update afterwards.

#54 @poena
8 weeks ago

Hi
As I see it @greenshady granted an exception, but then unfortunately he did not have the time to continue to do the full review.
And I still don't agree which is why I did not want to continue the review, -my opinions of the theme should not and will not prevent it from going live.

-I asked because @acosmin said they are "just styles", while I recall actual extra buttons and options in the editor, not only styles.

It is up to the reviewer from the accessibility team to decide if those changes needs to be reviewed or not.

#55 @greenshady
8 weeks ago

I want to be perfectly clear that I have granted no exception for the editor formatting options in this specific theme. This is a feature that we have allowed (and encouraged in some scenarios) over the years. Until such time that the TRT formally disallows such a feature, it is allowed in any theme.

#56 @webmandesign
8 weeks ago

Hi @poena,

@acosmin said they are "just styles", while I recall actual extra buttons and options in the editor, not only styles.

Please see the screenshot below. As you can see I'm only adding the "Formats" dropdown (which is actually called styleselect button in TinyMCE), and I've also enabled "Page Break" button (the wp_page, not sure why this button is no longer enabled by default in WordPress). No custom buttons are added to TinyMCE by the theme.

http://easycaptures.com/fs/uploaded/1195/8706002447.jpg

#57 @webmandesign
8 weeks ago

Sorry, I've just figured out there are more previous comments from you guys. Here is my answer to @poena's #comment:49:

  • Keyboard Navigation -did not pass
  • Headings -did not pass

Both of these issues were fixed in theme update 0.9.6. But indeed, I haven't received a theme review since from anybody in TRT as far as I'm concerned. So, my fixes for @poena's list (#comment:3) were not checked by you guys yet.

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

#58 @acosmin
8 weeks ago

I don's see anything wrong with #comment:56

The code generating those styles are also ok.

If no one has anything to add, I think the ticket can continue with a full a11y review and after that I can mark it live.

@webmandesign you'll need to update before it goes live :) but it shouldn't have any impact on a11y review.

@joedolson could you please add this on your list. Thanks.

Last edited 8 weeks ago by acosmin (previous) (diff)

#59 @acosmin
8 weeks ago

@webmandesign One last thing, includes\plugins\one-click-demo-import\class-one-click-demo-import.php L112-113

You can't hotlink to external sources, it would be ok if you include those files in the theme folder.

Please consider fixing this, thanks!

#60 @webmandesign
8 weeks ago

Hi @acosmin,

Thanks for update. We have actually discussed the demo content import during the initial theme review. Please check that conversation.

As the WebMan Templates plugin was accepted to WPORG plugins repository, I will post theme update soon.

Thanks and regards,

Oliver

#61 @themetracbot
8 weeks ago

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

Reykjavik - 0.9.12

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 -
Author URL - https://www.webmandesign.eu/

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

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

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

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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

  • RECOMMENDED: Could not find the file readme.txt in the theme. Please see Theme_Documentation for more information.

#62 @acosmin
8 weeks ago

@webmandesign We'll discuss the linking to outside sources for demo files in tomorrow's team meeting (on Slack), you're more than welcome to join and tell us your opinion.

#63 @webmandesign
8 weeks ago

@acosmin sure, thanks for letting me know. Just to confirm, it will be at 17:00 UTC in the #themereview channel as stated at https://make.wordpress.org/themes/ ?

#64 @acosmin
8 weeks ago

Yes, that's correct.

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


8 weeks ago

#66 @sami.keijonen
8 weeks ago

I did quick accessibility-ready review and overall good job on accessibility.

Headings hierarchy was only blocker that I'd fix.

Keyboard Navigation

  • Pass

Controls

  • Pass
  • Note: VoiceOver announces "Downwards arrow" in mobile sub-menu links (content: '\21B3\2002';).
  • If you can use SVG as an arrow, then go with that.
  • Pass
  • I was thinking should 3 skip links be inside a list. Multiple skip links

are fine but I advice not to add more. Skip links is about reducing the clutter

Forms

  • Pass

Headings

  • In blog and archive pages there is this kind of markup:
<div class="intro-inner">
	<h2 class="screen-reader-text">Introduction</h2>
    <h3 class="page-title h1 intro-title">Blog</h3>
</div>
  • And then again this markup:
<header class="page-header">
	<h1 class="page-title screen-reader-text">Blog</h1>
</header>
  • Use the latter only. There is no reason the repeat same information twice. Visually it can be same as the first markup. But I'd drop "Introduction"-wording. It's not added by the site owner.
  • I was also confused about <h2> inside <header> in blog and single view. <h2 class="screen-reader-text">Lorem Ipsum – Theme reviews</h2>. It kind of breaks heading hierarchy and repeats the title. I'd remove it.
  • Note: Same goes with <h2> headings in navigation but navigations needs label anyways. See below.

ARIA Landmark Roles

  • Pass
  • Note: one option is to use aria-label in navigation if you don't want to use <h2>.

Link Text

  • Pass

Contrasts

  • Pass

Images

  • Pass.
  • Note: Consider using SVG icons everywhere. In some cases you might need to inject the SVG via JS.

Media

  • Pass

Screen Reader Text

  • Pass

#67 @themetracbot
7 weeks ago

  • Summary changed from THEME: Reykjavik – 0.9.12 to THEME: Reykjavik – 0.9.13

Reykjavik - 0.9.13

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 -
Author URL - https://www.webmandesign.eu/

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

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

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

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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

  • RECOMMENDED: Could not find the file readme.txt in the theme. Please see Theme_Documentation for more information.

#68 @webmandesign
7 weeks ago

Hi @sami.keijonen,

Thank you very much for such quick accessibility review! I've updated the theme to version 0.9.13 and fixed the issues reported. Here are my comments:


Controls / VoiceOver

Thanks for spotting this, I've fixed this using CSS speak: none; as those arrows are added for submenu items on mobile navigation only.


Skip Links

I've actually added those additional skip links after reading some article about good accessibility practices with examples. I don't really have the article link any more, unfortunately. I've made multiple skip links into list as suggested.


Headings / Page title

This is a bit tricky to execute as ideally the Intro section would be part of the #primary content section. But due to design decision, I have to output Intro section before the #primary container.

I've fixed the issue to use this logic now:
If the Intro section is displayed, use H1 in there (except on front page, where we output H2) and do not output any page/post/archive title in #primary content area.
If the Intro section is not displayed, output H1 as page/post/archive title in #primary content area.

Priority for this logic is to output only one H1 (which was also the case before) and not duplicating page/post/archive title. Hope that helps ;)


Headings / H2 in website header

This was intended as HTML document title as without it the W3C Validator complained in the past. But when I check the code with W3C validator now, it is perfectly OK with no heading in there. So I've went ahead and removed it.


Headings / Navigation

I wasn't actually sure what would happen if I just use aria-label on navigations. As far as I know, with assistive technology you can navigate the website using headings list. I'm not sure aria-label would be listed there and I wanted to make all navigations easily accessible so user can jump to any of those using headings list.

What do you say, will aria-labels be accessible the same way as headings list? (I've went ahead and removed the H2 headings from <nav>s and replaced them with aria-labels.)


Images / SVG icons

Well, I was considering this, but it would really mean adding some of those SVGs with help of JS and I didn't want to go that way. If you don't mind I will stick to the currently implemented way. At least for now, while I can come up with something better in the future.


Other than that I've updated custom widgets enhancements to WP 4.9 code.

Thanks and regards,

Oliver

#69 @sami.keijonen
7 weeks ago

Accessibility ready review is now approved.

Notes below.

  • Note that speak: none; doesn't work for all AT and screen readers.
  • There can be several skip links but skip link main point usually is skipping to main content without extra clutter.
  • I think headings are fine now.
  • Screen reader users can navigate using landmarks also, <nav> is one of them. And aria-label gives them unique name. For example now VoiceOver announce Primary menu navigation or Secondary menu navigation and move to that menu directly.
  • Twenty17 is one good source how to use SVG icons.

#70 @webmandesign
7 weeks ago

Hi @sami.keijonen,

Thank you for the review and helpful information provided!

I'll go ahead and improve the mobile submenu arrow accessibility so I don't have to use CSS speak: none;. And will check the TwentySeventeen for more thorough SVG icons support.

Regards,

Oliver

#71 @acosmin
7 weeks ago

@webmandesign Congrats on the accessibility review.

The only issue now is removing those hotlinks :)

#72 @themetracbot
7 weeks ago

  • Summary changed from THEME: Reykjavik – 0.9.13 to THEME: Reykjavik – 0.9.14

Reykjavik - 0.9.14

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 -
Author URL - https://www.webmandesign.eu/

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

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

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

History:

Ticket Summary Status Resolution Owner
#44682 THEME: Reykjavik – 0.9.4 closed closed-newer-version-uploaded alkesh7
#46546 THEME: Reykjavik – 0.9.14 closed live acosmin

(this ticket)

#47776 THEME: Reykjavik – 1.0.0 closed live themetracbot
#47834 THEME: Reykjavik – 1.0.1 closed live themetracbot
#48212 THEME: Reykjavik – 1.0.2 closed live themetracbot
#48228 THEME: Reykjavik – 1.0.3 closed live themetracbot
#48259 THEME: Reykjavik – 1.0.4 closed live themetracbot


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

  • RECOMMENDED: Could not find the file readme.txt in the theme. Please see Theme_Documentation for more information.

#73 @webmandesign
7 weeks ago

Hi @acosmin,

Thanks! Also for reminder ;) Here are changes for theme version 0.9.14:

  • Adding demo XML and WIE files into the theme includes/starter-content folder (Message for Theme Review Team: Please, reconsider this for the future. The theme does not use any hot link in my opinion. It is only providing configuration for a plugin. And it is up to theme user to install and activate the plugin and to click the import button afterwards - it is a fully conscious decision and can not be done "by mistake". Besides I really don't see a point in including a theme demo XML and WIE files within the theme folder. Thanks for consideration!)
  • Preventing mobile Apple devices double tap on links issue
  • Improving mobile submenu indentation marker accessibility styles
  • Improving responsive content vertical margins
  • Preventing enqueuing style.css when not using a child theme
  • Updating theme documentation URL

Regards,

Oliver

#74 @acosmin
7 weeks ago

  • Resolution set to live
  • Status changed from reviewing to closed

@webmandesign Congrats, your theme should be live shortly!

#75 @webmandesign
7 weeks ago

@acosmin Thank you for making the theme live! :)

And thank you all for theme review and help provided!

Note: See TracTickets for help on using tickets.