-
Notifications
You must be signed in to change notification settings - Fork 42
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
Updates for ultimate-addons-for-gutenberg (now Spectra) #320
Open
diiegopereira
wants to merge
1
commit into
master
Choose a base branch
from
compsupp-6801
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diiegopereira, I saw your comment in
compsupp-6801
, that the Spectra plugin has a local WPML config file.Beware that if we override the local file, we may loose some local configurations if it was not synced on the remote one.
Can you make sure the local and remote config files are in sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@strategio it seems that we have several differences between our remote file and the one included in the plugin: https://www.diffchecker.com/ArriH1zI/
I was trying to manually update the remote file by adding the differences to it. But I found some parts that can generate conflicts. E.g. in the remote file, the string is registered using xPath, but in the plugin file, it is registered using "key" (or a different xPath that belongs to the same string).
What would be the correct way to sync both files? Should we use what's included in the plugin, which looks more up-to-date? Is there a problem with a string being registered twice (key + xPath, or 2 different xPath for the same string)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diiegopereira, let me share some elements:
String IDs (i.e. strings names) are based on the block name and the text content, we make a hash of that (see here). In short, strings are unique to block type and content. If we have twice the same content inside a block, we'll register only 1 string. So we don't risk to have duplicated strings or wrong string IDs inside blocks (note that this is not true for other page builders integration).
If you see that the plugin's local XML config looks more up-to-date than the remote one, it means that the author is maintaining his config (even not fully). At least, he probably does that more than us.
With all that said, I think we have 2 options:
Take real ownership of the config: First, we need to merge the local config into the remote one (as you started to do), and then inform the author that the remote configuration will now take precedence. If he wants to change/add blocks in the future, he will need to open a PR here.
Let the author take care of his config, and inform him about the issue we found (and perhaps ask him for a campaign of re-testing on his plugin).
I'll let you check what could be the best option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @strategio! I think we should contact the author and discuss the best option. Let's continue on youtrack for now.