WordPress.org

Make WordPress Themes

Opened 7 months ago

Last modified 6 days ago

#43185 reviewing theme

THEME: Aguafuerte – 1.0.7

Reported by: acalfieri Owned by: _smartik_
Priority: previously reviewed Keywords: theme-aguafuerte accessibility-ready
Cc: anaclaudia.alfieri@…

Description

Aguafuerte - 1.0.2

Aguafuerte is a theme designed to be fully responsive by taking the mobile-first approach. It features the full range of post formats, two menu locations, one sidebar and three footer widget areas.

Theme URL - http://ebyb.eu/aguafuerte
Author URL - http://ebyb.eu

Trac Browser - https://themes.trac.wordpress.org/browser/aguafuerte/1.0.2

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

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=aguafuerte/1.0.1&new_path=aguafuerte/1.0.2

History:

Ticket Summary Status Resolution Owner
#31408 THEME: Aguafuerte – 1.0.1 closed not-approved greenshady
#43185 THEME: Aguafuerte – 1.0.7 reviewing _smartik_

(this ticket)


https://themes.svn.wordpress.org/aguafuerte/1.0.2/screenshot.png

Change History (49)

#1 @joyously
5 months ago

Here is some user feedback to consider before your review.

  • The menu needs to have a fallback for when no menu is selected. The default value of fallback_cb parameter of wp_nav_menu() works really well, and it's core functionality.
  • The mobile menu doesn't handle window size changes correctly. If I start with a wide window and make it narrower, the mobile menu appears with the submenu arrows hidden. If I then make it wider again, the desktop submenu arrows appear but do nothing, while the mobile icon disappears. So I make it narrow again to get the X to show. However, if I load the page while the window is narrow, the mobile submenu arrows appear. Making the window wide then shows both arrow types on hover, but the X is gone. So I make it narrow to get the X, close the menu, and when it is wide again the mobile arrows are on the desktop menu (both arrows on hover).
  • As I make the window narrower, a scrollbar appears around 865px.
  • On a lot of desktop window sizes, everything is skinnier than it needs to be, with a lot of whitespace that could be used to make that content wider. Since you are using box-sizing: border-box, you should limit the use of padding, and just use margins. (There are some big paddings being used.)
  • Static Pages should show comments.
  • The blog page is showing the user content centered. Excerpts can have HTML tags, so should not be centered. (In general, centered text is more difficult to read and a lot of markup looks bad centered, so it's best to not center user content unless the user chooses it.)
  • Floats are not being cleared properly after the content area.

https://s11.postimg.org/f82jmem6r/floats-not-cleared.jpg

  • The style for floated images looks like pulling them farther left or right than the rest of the content. (I have made a design similar to this.) But floated images with captions are not styled the same way.
  • blockquote is styled as italic, but can have nested <em> and <cite> tags which are indistinguishable. It really does not need italics, since it has a lighter text color and a left bar.
  • The space between paragraphs feels to big for the font size.
  • The <q> tag is unstyled (default removed).
  • Links in comments look like regular text.
  • You might want to hide the menu, comment form, widget areas for the print styles.
  • Search results should not show the date of static Pages.
  • See Proper Copyright/License Attribution for Themes to list all resources used (PHP, JS, CSS, fonts, images).

#2 @themetracbot
5 months ago

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

#3 @djrmom
5 months ago

  • Keywords changed from theme-aguafuerte, accessibility-ready to theme-aguafuerte accessibility-ready
  • Owner tbangarayya deleted

This is being returned to the new queue again, as there has been no response yet from the reviewer. A reviewer should make an initial comment in ticket within 24 hours of being assigned, after which both reviewer and author should always communicate within 7 days.

If you are the reviewer and able to do this review, please carry on and request you get added back in Slack #themereview or you can take on another review when you have time again.

#4 @themetracbot
5 months ago

  • Owner set to swapnild

#5 @swapnild
5 months ago


# DESIGN

  1. When there is no menu selected then it fallback to the all pages. - https://goo.gl/sP5wLc

# CUSTOMIZER

  1. Scroll in customizer - https://goo.gl/jw6a1x
  2. Tagline color is not working - https://goo.gl/eN8zZv

# NS THEME CHECK
1.

FILE: /var/www/html/astra-zip.sharkz.in/public_html/wp-content/themes/aguafuerte/comments.php
--------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------
 26 | ERROR | Missing singular placeholder, needed for some languages. See
    |       | https://codex.wordpress.org/I18n_for_WordPress_Developers#Plurals
--------------------------------------------------------------------------------------------------------------

#6 @acalfieri
5 months ago

Hi @swapnild,

I'll upload a new version during the weekend. Thank you!

#7 @swapnild
5 months ago

Okay @acalfieri

#8 @themetracbot
5 months ago

  • Summary changed from THEME: Aguafuerte – 1.0.2 to THEME: Aguafuerte – 1.0.3

Aguafuerte - 1.0.3

Aguafuerte is a theme designed to be fully responsive by taking the mobile-first approach. It features the full range of post formats, two menu locations, one sidebar and three footer widget areas.

Theme URL - http://ebyb.eu/aguafuerte
Author URL - http://ebyb.eu

Trac Browser - https://themes.trac.wordpress.org/browser/aguafuerte/1.0.3

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

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=aguafuerte/1.0.2&new_path=aguafuerte/1.0.3

History:

Ticket Summary Status Resolution Owner
#31408 THEME: Aguafuerte – 1.0.1 closed not-approved greenshady
#43185 THEME: Aguafuerte – 1.0.7 reviewing _smartik_

(this ticket)


https://themes.svn.wordpress.org/aguafuerte/1.0.3/screenshot.png

#9 @acalfieri
5 months ago

Hi @swapnild,

I just uploaded a new version.

  • The menu issue is solved now.
  • I don't see the scroll you see in my browsers. The tagline not changing colour is a design decision. Only the name of the site changes colour.
  • The singular placeholder is in place. I don't know why the theme check shows an error.

Thank you very much for your time!

#10 @swapnild
5 months ago

@acalfieri

I will check the latest update on weekend.

#11 @djrmom
5 months ago

@swapnild, are you still able to do this review? If not I can reassign. Thanks.

#12 @swapnild
5 months ago

Sorry to say @djrmom I can not able to continue this review you can reassign. Thanks

Last edited 5 months ago by swapnild (previous) (diff)

#13 @djrmom
5 months ago

  • Owner swapnild deleted

Returning to the queue for a new reviewer.

#14 @themetracbot
5 months ago

  • Owner set to _smartik_

#15 @_smartik_
5 months ago

1. Define the JS in strict mode:

(function( $ ){
  "use strict";

  // All your javascript goes here
})( jQuery );

2. Submenu background is missing:

http://i.imgur.com/YavT07j.jpg

3. Shouldn't the content height be equal with the sidebar?

http://i.imgur.com/r8MJeUP.jpg

4. The level 2 submenu is visible on large screens. You probably should not use the CSS left: -1000% but display: none instead.

http://i.imgur.com/O4nBYJg.jpg

Last edited 5 months ago by _smartik_ (previous) (diff)

#16 @themetracbot
5 months ago

  • Summary changed from THEME: Aguafuerte – 1.0.3 to THEME: Aguafuerte – 1.0.4

Aguafuerte - 1.0.4

Aguafuerte is a theme designed to be fully responsive by taking the mobile-first approach. It features the full range of post formats, two menu locations, one sidebar and three footer widget areas.

Theme URL - http://ebyb.eu/aguafuerte
Author URL - http://ebyb.eu

Trac Browser - https://themes.trac.wordpress.org/browser/aguafuerte/1.0.4

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

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=aguafuerte/1.0.3&new_path=aguafuerte/1.0.4

History:

Ticket Summary Status Resolution Owner
#31408 THEME: Aguafuerte – 1.0.1 closed not-approved greenshady
#43185 THEME: Aguafuerte – 1.0.7 reviewing _smartik_

(this ticket)


https://themes.svn.wordpress.org/aguafuerte/1.0.4/screenshot.png

#17 @acalfieri
5 months ago

Hi @_smartik_,

Thank you for your time!

  1. I don't think "use strict" is a requirement in the custom js files.
  1. I don't know why you have that problem, but the <a> element has a background color on hover, as you can see in my demo site.
  1. I think it is solved now.
  1. That issue doesn't happen in my configuration either. In any case, I can not use display:none due to the accessibility requirements.

Cheers!

#18 @_smartik_
5 months ago

1. Defining JS in strict mode is just a friendly advice. Anyway is not a requirement. You can read more here:

https://stackoverflow.com/a/1335881/1050262


2. The submenu background is transparent. Please fix this before you resubmit:

http://i.imgur.com/hoBSuvt.jpg


3. Please fix the submenu. As I said, this is visible only on large screens. See the screenshot:

http://i.imgur.com/f47rvDh.jpg


You are almost here. Just fix these issues.

#19 @acalfieri
5 months ago

Dear @_smartik_

Could you please send me your operating system and the browser you are using so I can reproduce the issue with the transparent menu? In my OS and browsers I don't see it like this because, as I said before, the <a> element, which is over the <ul> element, has a background.

Regarding the last issue, I cannot change to display: none, because that is not acceptable in themes that have the tag accessibility-ready.

Kindly,

#20 @_smartik_
5 months ago

@acalfieri , the theme is buggy in all Chrome, Firefox and Edge browsers.
In Firefox, the arrow gets a bg color and the submenu has the same problems from Chrome.
In Microsoft Edge an horizontal scrollbar is added.

OS: Windows 10.

Also, the site navigation is not visible on small screen sizes.

In firefox:
http://i.imgur.com/0y8R4hj.jpg

On small screens:
http://i.imgur.com/hetzEpo.jpg

#21 @acalfieri
4 months ago

Hi @_smartik_,

I checked the theme in Windows 7 and 8 (IE, Firefox and Chrome) and I only see some problems in IE. I checked it in Ubuntu 14 with Firefox and Chrome and it works well. I also checked it in Mac (Safari, Firefox and Chrome) and everything is fine. It works well also in mobile both in Android and iOS. It is not totally ready for iPad yet.

Unfortunately, I don't have access to Windows 10 and Edge. Nevertheless, design issues are only relevant in the case of this theme for accessibility reasons. I think we need to ping @joedolson, @poena or @sami.keijonen to see how we can continue.

Kindly,

#22 @poena
4 months ago

Hi
I tested version 1.0.4 On PC, win10, in chrome, edge and firefox and I can see the background issue in all 3 browsers, but only when no menu is selected. So the problem is with the styling of the fallback menu.

Check style.css L2830, it looks like it only changes the background for menu-item-has-children, not page_item_has_children.

#23 @themetracbot
4 months ago

  • Summary changed from THEME: Aguafuerte – 1.0.4 to THEME: Aguafuerte – 1.0.5

Aguafuerte - 1.0.5

Aguafuerte is a theme designed to be fully responsive by taking the mobile-first approach. It features the full range of post formats, two menu locations, one sidebar and three footer widget areas.

Theme URL - http://ebyb.eu/themes/aguafuerte/
Author URL - http://ebyb.eu

Trac Browser - https://themes.trac.wordpress.org/browser/aguafuerte/1.0.5

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

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=aguafuerte/1.0.4&new_path=aguafuerte/1.0.5

History:

Ticket Summary Status Resolution Owner
#31408 THEME: Aguafuerte – 1.0.1 closed not-approved greenshady
#43185 THEME: Aguafuerte – 1.0.7 reviewing _smartik_

(this ticket)


https://themes.svn.wordpress.org/aguafuerte/1.0.5/screenshot.png

#24 @acalfieri
4 months ago

Dear @_smartik_ and @poena,

I've just uploaded a new version with @poena's suggestion. Let's see if it works.

Kindly,

#25 @_smartik_
4 months ago

Looks OK, now.

The only remaining thing is the navigation not being visible on small screens:
http://i.imgur.com/hetzEpo.jpg

#26 @themetracbot
4 months ago

  • Summary changed from THEME: Aguafuerte – 1.0.5 to THEME: Aguafuerte – 1.0.6

Aguafuerte - 1.0.6

Aguafuerte is a theme designed to be fully responsive by taking the mobile-first approach. It features the full range of post formats, two menu locations, one sidebar and three footer widget areas.

Theme URL - http://ebyb.eu/themes/aguafuerte/
Author URL - http://ebyb.eu

Trac Browser - https://themes.trac.wordpress.org/browser/aguafuerte/1.0.6

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

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=aguafuerte/1.0.5&new_path=aguafuerte/1.0.6

History:

Ticket Summary Status Resolution Owner
#31408 THEME: Aguafuerte – 1.0.1 closed not-approved greenshady
#43185 THEME: Aguafuerte – 1.0.7 reviewing _smartik_

(this ticket)


https://themes.svn.wordpress.org/aguafuerte/1.0.6/screenshot.png

#27 @acalfieri
4 months ago

Hi @_smartik_,

Let's see if it works now. Try to reload the page when you change the size of the browser or try it on mobile devices, please.

Kindly,

#28 @_smartik_
4 months ago

Please test the changes you make.
The navigation is not still visible. All what you did in v1.0.6, was to change just one line from CSS that has zero effect to the actual problem.

If you continue to resubmit without changes, I'll have to mark as unapproved.

#29 @acalfieri
4 months ago

Dear @_smartik_

As I said before, unfortunately I have not the possibility of testing the theme in your operating system, your display resolution and/or your browsers. In all the configurations I test the theme (and as you can see above there are quite a lot), the problem does not appear and the menus work beautifully, so I'm trying to guess what it can be and trying to solve it the best I can.

The issue, by the way, is not a reason for not approving the theme, but if you are able to provide the solution, I'll be happy to implement it.

Kindly,

#30 @_smartik_
4 months ago

Your demo site looks OK to me. Obviously there is a difference between the theme used on your demo site and this.
The navigation menu button is visible on demo site, but I can't get it working on my localhost installation.

Please try this theme on a fresh WP site.

https://www.dropbox.com/s/xbxaxmzzghg4pge/screenshot-2.jpg?dl=0

Last edited 4 months ago by _smartik_ (previous) (diff)

#31 @acalfieri
4 months ago

Hi @_smartik_

The theme installed on my demo site is exactly the same you are reviewing. I update the files every time I make a change. Maybe a problem with your local installation?

Cheers,

#32 @joyously
4 months ago

Hi, I just thought I would mention that I tried the latest version of the theme and if I keep the fallback menu, there is no menu icon on a narrow window. If I change to use a custom menu, there is a menu icon on a narrow window.
Also, users should not have to "reload the page when you change the size of the browser or try it on mobile devices". The theme should make sure it works regardless. One way to make sure the latest stylesheet is used is to put a version number on it in the enqueue call. Switching from wide to narrow should just work (like rotating your phone or tablet).

#33 @acalfieri
4 months ago

@_smartik_, @joyously

Dear both,

I don't know what your idea of responsive design is, but for me it's that the content of a site can be accessed comfortably on a mobile device and also in a desktop setting. Responsive doesn't mean that everything has to be perfect when the user starts playing with the size of the browser just for the sake of it.

In this case there is only one cut point: 881px. If the size of the window is below that, a button appears and this button serves to open a vertical menu; inside the menu there are small buttons to open the submenus. If the size is above 881px, the menu is horizontal and the submenus open on hover.

If one starts playing with the size of the browser (and it seems you both like to do it), that behaviour only happens if one reloads the window, because I only wrote the code for the load event and not for the resize event.

Regarding the fallback menu issue, the function wp_nav_menu() has a parameter 'fallback_cb', which default value is 'wp_page_menu'. This means that if the developer does not do anything and there is no menu set in the location, the menu will show all the pages of the site. If the site has not any page (only posts, say), the menu will not show. The developer could choose to set the 'fallback_cb' to 'false' and in this case no menu will show, either. You can read more about the function here https://developer.wordpress.org/reference/functions/wp_nav_menu/

If you look at header.php, lines 38-40 you will see that there is a small condition: the button only appears it there is a menu in one of the two menu locations of the theme: either in 'primary', in 'social' or in both. If no menu is set, no button.

I hope you can find this useful.

Kindly,

#34 @joyously
4 months ago

Responsive doesn't mean that everything has to be perfect when the user starts playing with the size of the browser just for the sake of it.

Yes it does, because phones and tablets can be rotated, changing the size of the window dynamically. It is unrealistic to expect the user to have to reload the page just because they rotated the device.

Regarding the fallback menu issue,

The issue is not whether you use the fallback menu or not (I always am pushing for authors to supply that fallback). The issue is that the mobile menu icon does not show up when the fallback menu is used, so those users on a phone will have no menu. If the admin sees that fallback on a desktop, they will assume it will be there for the mobile view also, as it should be. I just wanted to clear up the disagreement as to whether the theme had the menu icon or not, since you both only saw it one way and they did not match...

#35 @rabmalin
3 months ago

@acalfieri What is the status of this ticket? Is this complete from your side and waiting for a11y review?

#36 @acalfieri
3 months ago

Hi @rabmalin,

Yes.

#37 @dwigpro
3 months ago

@acalfieri According to @joyously's comment you have to fix this and then the theme will get approved:

The issue is that the mobile menu icon does not show up when the fallback menu is used, so those users on a phone will have no menu. If the admin sees that fallback on a desktop, they will assume it will be there for the mobile view also, as it should be.

#38 @acalfieri
3 months ago

@dwigpro,

This theme can not be approved until it passes an accessibility review. Besides, @joyously comments are not a reason for not approving a theme and in any case she is not the reviewer of this theme.

I don't know why you are commenting in this ticket, but in case you and @_smartik_ are the same person, I think you need to know that having multiple user accounts is absolutely not allowed.

Kindly,

Ana

#39 @rabmalin
3 months ago

I found following issues in the theme. Please check those.

Issues

  • REQUIRED: Use the_archive_title() to display archive title in archive templates and use the_archive_description() to display archive description. Use appropriate hook if you want to customize. Eg, category.php. Check other archive templates also.
  • REQUIRED: Remove search-form from add_theme_support( 'html5' ) as you are customizing it yourself.
  • REQUIRED: Localize object screenReaderText should be prefixed.
  • REQUIRED: Even if images used are yours, you need to declare copyright and license. Please update readme.txt
  • REQUIRED: Removing core settings is not allowed. Use alternative approach to disable header text.
    $wp_customize->remove_control('display_header_text');
    
  • REQUIRED: $wp_customize->add_section('Blog Navigation') - Section ID should be slug-like string. Otherwise it may create problem.

#40 @themetracbot
3 months ago

  • Summary changed from THEME: Aguafuerte – 1.0.6 to THEME: Aguafuerte – 1.0.7

Aguafuerte - 1.0.7

Aguafuerte is a theme designed to be fully responsive by taking the mobile-first approach. It features the full range of post formats, two menu locations, one sidebar and three footer widget areas.

Theme URL - http://ebyb.eu/themes/aguafuerte/
Author URL - http://ebyb.eu

Trac Browser - https://themes.trac.wordpress.org/browser/aguafuerte/1.0.7

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

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=aguafuerte/1.0.6&new_path=aguafuerte/1.0.7

History:

Ticket Summary Status Resolution Owner
#31408 THEME: Aguafuerte – 1.0.1 closed not-approved greenshady
#43185 THEME: Aguafuerte – 1.0.7 reviewing _smartik_

(this ticket)


https://themes.svn.wordpress.org/aguafuerte/1.0.7/screenshot.png

#41 @acalfieri
3 months ago

Dear @rabmalin,

  • I filtered the_archive_title().
  • I removed search-form from add_theme_support('html5').
  • screenReaderText is now prefixed.
  • I declared the copyright and licence of my own images.
  • I know that removing core settings is not allowed, but I would like to split the "Display Site Title and Tagline" into "Display Site Title" and "Display Tagline" and I couldn't find a better solution without making it more complicated. I hope it's OK.
  • I changed the section ID as you suggested.

Kindly

#42 @rabmalin
3 months ago

There is header-text parameter in Custom Header. You can check documentation https://developer.wordpress.org/themes/functionality/custom-headers/ Keep false for that parameter.

#43 @_smartik_
3 months ago

  • Status changed from reviewing to approved

Looks OK to me, now.

#44 @rabmalin
3 months ago

  • Status changed from approved to reopened

#45 @rabmalin
3 months ago

  • Status changed from reopened to reviewing

Reopening ticket. This theme does not seem to have gone through a11y review.

Pinging @joedolson @davidakennedy for a11y review.

#46 @joedolson
3 weeks ago

1) Keyboard Navigation - Fail - no :focus state on mobile navigation toggle.

2) Controls - Pass. (Note: aria-expanded should not be attached to the header element, only to the button.)

3) Skip Links - Pass

4) Forms - Pass

5) Headings - Pass

6) ARIA Landmark Roles - Fail. Please do not use the aside element for individual widgets, only for the wrapper surrounding your widget region.

7) Link Text - Pass.

8) Contrasts - Fail. But it's hard to assess. Because you're using light colored text over a semi-transparent background color covering an image, it's almost impossible to provide a definitive answer to this. However, in *most* situations, the #888 color will fail, and needs to be darkened at least to #767676, and preferably darker, due to the variability of the context.

9) Images - Pass

10) Media - Pass

11) Not Allowed - Pass

#47 @acalfieri
3 weeks ago

Hi @joedolson,

I'll update during the weekend.

Thank you!

#48 @joedolson
6 days ago

Hi, @acalfieri - Still planning to update this?

#49 @acalfieri
6 days ago

Hi @joedolson,

Yes! But, please, if you could wait a week more I would be grateful!

Cheers,

Ana

Note: See TracTickets for help on using tickets.