Some Minor Updates #11
Conversation
|
hey @B-07 thanks for the pull request. I wasn't following the last a few theme review meetings and I couldn't find the new requirement. Would you please share it here? Thanks in advance! |
|
There should be no period for file headers' summary as per the WP PHP Documentation standards. |
|
Okay, if we say we are using Summary, that's fine. We can accept that. However, I don't believe we have a decent summary and description of these files/functions, which is something we can work on in the future. However, thanks for your contribution! We are happy to accept more pull requests in the future, so feel free to suggest other improvements. I'll ask @xavortm, one of the maintainers of the starter theme, who's gonna provide more feedback. |
| @@ -33,7 +33,7 @@ | |||
| '<span>' . get_the_title() . '</span>' | |||
| ); | |||
| ?> | |||
| </h2> | |||
| </h2><!-- .comments-title --> | |||
There was a problem hiding this comment.
I am using emmet for writing the HTML markup. These and comments are added automatically through it's filters. In this case exactly we are not adding end comments - that being the line HTML elements. You can see more about this in here - http://docs.emmet.io/filters/ and in here https://github.com/sergeche/emmet-sublime/issues/225
It's okay to leave it as it is for now, but do not add more like this one in the future as the whole theme is full of such elements and if we add just for one it will less systematic.
inc/custom-header.php
Outdated
|
|
||
| /** | ||
| * Sample implementation of the Custom Header feature. | ||
| * Sample implementation of the Custom Header feature |
There was a problem hiding this comment.
Did you delete this because of the highlighting in GitHub? If that's the case there's no need to remove it since it will be of help for some of the developers for quick access to working snippet. Can you please revert this before we merge the PR?
|
Hey @B-07 I did leave you a few comments, please take a look at them and if fixed we will merge the PR. As Stanko mentioned, please try to keep such changes like changing comments in the same commit, it's easier to keep track of. If you work on a couple o bigger changes it's fully understandable to separate them in different commits, but this is not the case. Anyway, I am very happy that you took your time to work on our project, your PR did make me smile when I saw it! Cheers mate, we are waiting for updates :) |
|
@xavortm Thank you :) yes I know it's not best practice to use several commits on one issue, but I didn't think I would find so many of them :) Anyways, I changed files as you said. Cheers! |
|
@xavortm @metodiew Hi, I'm working on making all changes within single commit. I have a question, please help me here. I also use tabs for indentation, and in sublime I set 1tab = 4 spaces. But in github it shows 8 spaces. I notices in your files it shows 4 spaces. Can you please tell me how to do that? Much appreciated :) |
|
Most likely this comes from 4 spaces (not tabs). We still have to get this standardization across all files set properly, but that's a todo for another day :) Don't worry about it, use regular tabs. GitHub's tabs are set by default to 8, you can't edit that. |
Removing periods from file headers, as described in wp guidelines (approved in _s too) and fixing some minor things.