View in #core on Slack
@Matt_Robinson: Who is ready for a PR review session?
Main focus, as last week, is:
https://github.com/ClassicPress/ClassicPress/pull/1051
@Viktor - you raise a few queries about arrays, are you satisfied that single line arrays can stay on one line?
@Viktor: Iām not an expert, your explanation and reasoning is good.
@Matt_Robinson: Happy with he translations as a future project too?
@Viktor: What are you referring to? I may have missed something.
@Matt_Robinson: There were some comments about using sprintf for translations and that is fine a valid, but we have not fully activated that rule check yet.
@Joy_Reynolds: Keep this PR to the basics, put other stuff on future PRs.
@Viktor: Oh yes. I think it was mentioned that the current set up allows URL changes to localized contentā¦ if we had any. For now, I think keeping it as is would be the best option.
@Joy_Reynolds: I have looked at each file. I revisited the ones with changes, but Iām not sure what Iām looking at in some of those. I think a couple of the WP changesets pulled in are out of scope for this PR. Not sure what you want to do with thatā¦
I didnāt try to run itā¦
@Matt_Robinson: The PRās pulled in were in the hope of fixing some of the issues raised. I am hoping we are now not too far away from something we can merge.
@Joy_Reynolds: Do you want to go through the remaining conversations?
@Matt_Robinson: I think thatād probably work best.
The src/wp-admin/custom-header.php file has a few issues but the entire thing is messy.
Itās a mixture of PHP, HTML and javascript all in one file so the indenting looks weird but passes testing. Readability isnāt too bad. Some of the HTML and JS could perhaps be indented burther but that will probably increase white space when rendered, that wonāt affect layout but will increase download size.
@Joy_Reynolds: I donāt see any comments on it, so I assume itās good.
@Matt_Robinson: Iām looking at them now in the Conversation tab - when itās overly long the middle section is compressed and must be manually loaded on GH.
If you are happy though I can just resolve them.
@Joy_Reynolds: I was in the Files tab, and I guess I have to expand the diff to see the conversationsā¦
@Matt_Robinson: Does it show in the Files tab if you filter for the specific file?
@Joy_Reynolds: I think all three of those were minor, so if it passes the CS, leave it.
@Matt_Robinson: Next is src/wp-admin/edit-form-advanced.php
The underscored variable use scope - I suspect it is underscored in an attempt to prevent global stomping.
@Joy_Reynolds: I think so too. Thatās part of why I prefer it inline. But the whole reason most of this is done is to match WP, so I guess leave it.
@Matt_Robinson: Next is src/wp-admin/includes/widgets.php - variables set far away from usage
@Joy_Reynolds: Yes, I would prefer to put the ternary into the original, instead of setting another variable 80 lines away.
@Matt_Robinson: Is that more for the $noform_class - there was a comment about other parameters that are all grouped higher up too.
@Joy_Reynolds: Iām referring to width and height for my comment.
What I donāt know, and shouldnāt have to know, is whether the original variable changes in those 80 lines.
@Matt_Robinson: The variable are currently all collected together in one block. We can consider refactoring the code so that variable are defined immediately prior to use. I suspect itās a hvit mirrored throughout WP though.
@Joy_Reynolds: I suspect it as well, but we already talked about some changes that were better off as ternaries on one line versus the expanded if statement over several lines. This one adds a variable assignment that is a ternary, so why not just put that into the original? Iām all for getting changes from WP, but not if they are dumb and make things harder.
@Matt_Robinson: So, height, width, an the whole set?
@Joy_Reynolds: I say if itās just a ternary assignment, moved far away, donāt do that. Put the ternary (with esc_attr) into the original.
What do you think?
@Matt_Robinson: Looking again, we can only really move the height and width, the other variables are processed to create the invisible widget data for dragging and then the live widget itself if placed. We can do it for those 2 easily though.
@Joy_Reynolds: OK. Thatās what my comment was about anyway.
@Matt_Robinson: Shall we do the same - inline ternary for the $noform_class as well?
@Joy_Reynolds: I think there are a bunch similar which we left, to match WP, so I guess not. They tended to group the PHP instead of put it near where it is used.
@Matt_Robinson: Okay, so for the width the code will be:
<input type="hidden" name="widget-width" class="widget-width" value="<?php echo esc_attr( isset( $control['width'] ) ? $control['width'] : '' ); ?>" />
Similar for the height - updated PR to follow shortly, local testing seems fine.
@Joy_Reynolds: I would put
<input type="hidden" name="widget-width" class="widget-width" value="<?php echo isset( $control['width'] ? esc_attr( $control['width'] ) : '' ); ?>" />`` actually, that's not quite right either. <input type=āhiddenā name=āwidget-widthā class=āwidget-widthā value=ā<?php echo isset( $control[āwidthā] ) ? esc_attr( $control[āwidthā] ) : āā; ?>ā />``
@Matt_Robinson: Updated PR will follow with that then.
Next is src/wp-admin/setup-config.php with the language direction changes.
@Joy_Reynolds: Iām not seeing a comment.
Oh, thatās because the comment was on the original commit and this is changed?
@Matt_Robinson: I am looking now and wondering if any of that is really needed, it seems CP handles it all well in the get_language_attributes() function.
@Joy_Reynolds: What I see is the default value of $dir_attr is empty, so when the page is LTR, there will be no space between attributes because the space was removed.
@Matt_Robinson: https://github.com/ClassicPress/ClassicPress/blob/412e793ca413609ca901adc2a86634172d5b0de1/src/wp-includes/general-template.php#L3607
<ClassicPress/src/wp-includes/general-template.php at 412e793ca413609ca901adc2a86634172d5b0de1 Ā· ClassicPress/ClassicPress Ā· GitHub | general-template.php>
It looks like it really doesnāt need local handling in addition to the function call.
@Joy_Reynolds: That seems correct to me.
Does it still need the leading space?
I think it does.
@Matt_Robinson: Yes, that needs putting back. I found an early CP commit that made some changes there so we donāt need it, manually reverting.
Next is a couple of long if blocks in src/wp-includes/widgets/class-wp-nav-menu-widget.php
@Joy_Reynolds: This is similar to the other ones. What do you think?
@Matt_Robinson: They need to be consistent - one excludes the word style, leave them defined higher up or inline them?
@Joy_Reynolds: I agree if itās left this way, the string should have the style attribute in it so it doesnāt come out style="".
@Matt_Robinson: Okay, Iāll leave in the current layout but clean up the consistency and spacing.
@Joy_Reynolds: But why are they doing it this way? There is a message telling the user there are no menus, create some. But it is hidden with inline style if there are menus. So is this changed dynamically when a menu is created?
@Matt_Robinson: I donāt know enough about this code to answer that.
@Joy_Reynolds: I mean, the one output when there are menus uses PHP to get the title etc., so itās not as if it could be done in JS. It just seems like a weird way to do things. Iād rather use a class to hide it, or not output it at all.
@Matt_Robinson: Iāve got to step away for about 15 minutes, will try to come back as I think we are almost there on this PR. And Iām away for the next 2 weeks too.
I have update $nav_menu_style to include the style declaration now. There do seem to be a lot of unnecessary spaces added in the code as it stands.
@Joy_Reynolds: I was revisiting the Contributing doc and it still doesnāt have anything about working on more than one PR, or testing multiple in a row, so can you tell me if the git checkout command is safe to use serially? (I think the PRs are all different branches, so it should be, but I donāt want to mess things up or have residual changes from a separate PR).
And before you disappear, can you suggest what can be done to move forward while you are gone?
@Matt_Robinson: git checkout will switch branches but you need to take care not to make any local changes as they may well migrate with you as you switch branch. So, itās fine but you should check git status or git diff first to ensure there are no inadvertent changes. If there are deliberate changes then commit them.
@Joy_Reynolds: OK. I think that belongs in the doc.
@Matt_Robinson: While Iām away for the next 2 weeks, any of the PRs in the 1.5.0 milestone can be reviewed:
https://github.com/ClassicPress/ClassicPress/milestone/16
Either leave a comment in the conversation or the community reviewed label can be added to the PR - then on my return we should hopefully be able to commit or rebase and commit them a bit faster.
@Laurence: How do you use git @Joy_Reynolds ? Do you use CLI or a GUI?
@Joy_Reynolds: Is there anything else needed for the Layout PR?
@Matt_Robinson: The changes to the src/wp-admin/nav-menus.php file in the layout PR with regards to links match upstream I think, there were some small layout improvements ready to commit from here.
@Simone_Fioravanti: I use to first create a local branch and then pull multiple PR with git pull.
@Joy_Reynolds: I was going to try to checkout the PR so I could run it to make sure it all worksā¦
@Matt_Robinson: And there are some references to use the core checked() function, something Iām not against but that will need some manual testing.
@Joy_Reynolds: I thought running those screens that had PRs backported and questions on would be best.
@Matt_Robinson: Agreed, only sometimes tracking down the places in admin where the change applies can be a challenge in itself!
Just 2 files left I think:
src/wp-admin/includes/media.php mentions using checked() - perhaps we can look to pick that up in a later PR
src/wp-admin/nav-menus.php has some long href definitions. The only way to improve those I think is to define a variable first and pass it in later. Or we just leave it.
@Joy_Reynolds: Do you have a preference?
@Matt_Robinson: I think we can look at checked() later more globally through the code.
The href definitions are counter intuitive really but readable and match upstream, Iād be inclined to leave for now at least.
@Joy_Reynolds: sounds good to me
So if I want to submit a PR for the directory, should I base it on develop or on this PR ?
@Matt_Robinson: Okay, Iāll push the commit I have fixing up what we have discussed. As far as I can tell we have now reviewed all issues raised and either accepted or corrected them.
If the branch can be tested while Iām away then if it all looks like it works the first order of business can be to merge on my return.
Iāve now pushed (hopefully final) changes to the Layout PR. Next meeting I can make is the 17th August - feel free to continue without me though 