Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update manage-locations.js #30933

Closed
wants to merge 1 commit into from
Closed

Conversation

Quintis1212
Copy link
Contributor

@Quintis1212 Quintis1212 commented Apr 17, 2021

Hi , I added label to manage location menu on "Navigation screen: regression of improved menu locations select placeholder #30330" issue from new branch

@kevin940726
Copy link
Member

There are still come conflicts, could you rebase the branch onto trunk :)?

@talldan
Copy link
Contributor

talldan commented Apr 19, 2021

I think the same thing happened as with the previous PR.

There's some info here about how to sync the version of trunk on your fork with the one in this repo:
https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/git-workflow.md

@getdave
Copy link
Contributor

getdave commented May 4, 2021

Hi @Quintis1212. How are things going? Do you think you will be able to sync your fork with the Gutenberg repo? Anything I can help with?

@Quintis1212
Copy link
Contributor Author

Hi @Quintis1212. How are things going? Do you think you will be able to sync your fork with the Gutenberg repo? Anything I can help with?

Hi @getdave ) I don't know what I must to do , my other branches are working fine

@getdave
Copy link
Contributor

getdave commented May 10, 2021

Hi @Quintis1212. Ok no worries. Your fork of Gutenberg is out of sync with the Core Gutenberg repo trunk. That's why we see packages/edit-navigation/src/components/header/manage-locations.js mentioned under Conflicting files on this PR page.

You need to pull in the changes from the trunk branch of the Core Gutenberg repo. To do this:

  1. Add the Core repo as the alias upstream - git remote add upstream https://github.com/WordPress/gutenberg.
  2. Pull the latest trunk from upstream - git fetch upstream trunk.
  3. Switch to your branch on your fork - git checkout 123-L.
  4. Rebase your branch with changes from upstream - git rebase upstream/trunk.
  5. Resolve conflicts.
  6. Once done then push back to your branch git push -u origin 123-L (this assumes your fork is called origin).

Note for step 6, if you get any errors saying your local branch may overwrite the remote you can try appending --force-with-lease to the end of the command (eg: git push origin 123-L --force-with-lease).

I hope that helps? Let me know how you get on. I'd love to help you get this landed.

@talldan
Copy link
Contributor

talldan commented May 11, 2021

@Quintis1212 I'd also recommend updating your fork's trunk branch up-to-date with Gutenberg's trunk branch. It's good to do this before creating a new topic branch that you plan to work on.

Github now has a handy feature to help with that. You can click this 'Fetch and merge' button for your fork:
Screenshot 2021-05-11 at 12 43 27 pm

And then the following will bring those changes to your local machine:

  1. git checkout trunk
  2. git pull

@getdave
Copy link
Contributor

getdave commented May 13, 2021

@Quintis1212 It looks like the file you are editing has moved to here:

{ value: 0, label: __( '-' ), key: 0 },
...menus.map( ( { id, name } ) => ( {
key: id,
value: id,
label: name,
} ) ),

You might find it easier to simply create a new PR based off of trunk and then make your changes again in the new file (packages/edit-navigation/src/components/inspector-additions/manage-locations.js).

I'll be happy to review and approve as soon as you get to it.

Let me know if I can help in any way. Thanks again.

@Quintis1212
Copy link
Contributor Author

Hello @getdave ,@talldan ) thank you for help ) I deleted my local gutenberg plugin and cloned gutenberg from my fork to open new PR without conflicts , but I have an error :
npm ERR! code EBADENGINE npm ERR! engine Unsupported engine npm ERR! engine Not compatible with your version of node/npm: [email protected] npm ERR! notsup Not compatible with your version of node/npm: [email protected] npm ERR! notsup Required: {"node":">=10.0.0","npm":">=6.9.0 <7"} npm ERR! notsup Actual: {"npm":"7.10.0","node":"v14.16.0"}
I must have the lower version of npm to run npm ci ?

@getdave
Copy link
Contributor

getdave commented May 17, 2021

Hello @getdave ,@talldan ) thank you for help ) I deleted my local gutenberg plugin and cloned gutenberg from my fork to open new PR without conflicts , but I have an error :
npm ERR! code EBADENGINE npm ERR! engine Unsupported engine npm ERR! engine Not compatible with your version of node/npm: [email protected] npm ERR! notsup Not compatible with your version of node/npm: [email protected] npm ERR! notsup Required: {"node":">=10.0.0","npm":">=6.9.0 <7"} npm ERR! notsup Actual: {"npm":"7.10.0","node":"v14.16.0"}
I must have the lower version of npm to run npm ci ?

At the moment Gutenberg requires npm less than 7.

I had the same issue so I temporarily downgraded npm to 6.9.0.

npm install -g [email protected]

@Quintis1212
Copy link
Contributor Author

Quintis1212 commented May 17, 2021

Thank you @getdave) Do you know who is working for allowing 7 and high npm ? I want to help

@getdave
Copy link
Contributor

getdave commented May 17, 2021

Thank you @getdave) Do you know who is working for allowing 7 and high npm ? I want to help

I don't specifically but if you ask in the core-js channel in WordPress.org Slack someone will probably be able to help.

@Quintis1212 Quintis1212 requested a review from getdave as a code owner May 17, 2021 16:09
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to approve this if we can update to remove the characters from inside the string to be translated.

@@ -34,7 +34,7 @@ export default function ManageLocations() {
labelPosition="top"
value={ menuLocation.menu }
options={ [
{ value: 0, label: __( '-' ) },
{ value: 0, label: __( '— Select a Menu —' ) },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ value: 0, label: __( 'Select a Menu' ) },
{ value: 0, label: __( 'Select a Menu' ) },

Looking at the i18n guidelines I'm not sure we should include the characters inside the translated string.

@getdave
Copy link
Contributor

getdave commented May 24, 2021

Hey @Quintis1212. Looks like this is nearly ready to land although there is now another conflict with the packages/edit-navigation/src/components/header/manage-locations.js file (same process as before). Happy to approve if we can address the final point in my code review.

I'd love to help you land this PR. Let me know if/how I can help.

@getdave
Copy link
Contributor

getdave commented Jun 3, 2021

@Quintis1212 Is there anything I can do to help you further here?

@Quintis1212
Copy link
Contributor Author

Quintis1212 commented Jun 3, 2021

Hi @getdave , I created new branch and new PR - #32423 , but it seems there is a conflict

@Mamaduka Mamaduka closed this Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants