-
Notifications
You must be signed in to change notification settings - Fork 5
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
Libs 8.0.0 release #412
Libs 8.0.0 release #412
Conversation
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.
Nice work 👏🏼
Left a few suggestions
Co-authored-by: Ivan Ramljak <[email protected]>
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.
What is the purpose of the setCache
empty file? Does it have to be committed?
Changed the getManifest
to getManifestDir
- how will this impact userland code? Is getManifest
used, for instance, in projects? If so, will updating to this version break this implementation?
If the answer is yes, the version of the forms should be updated to 3.0.0 notifying users of a BC break.
The same happened with some UtilsGeneralHelper
to Helpers
classes, but I think those are used internally, so they are not necessarily BC breaks.
Also, I've left a couple of suggestions regarding naming, and some potentially misnamed methods.
As these forms change, this will impact users in no way. As the addon has its own libs and we are not implementing any breaking changes to the user's implementation in the project, so I don't see any need to bump the forms to 3.0.0
tnx, will fix it |
So the changes in the blocks won't affect the existing blocks on the existing forms? I'm just wondering if this will have any impact on the custom blocks that can be added and are depending on the old utils. |
As the forms are now for the users to develop on like Boilerplate is, what ever we do on the internal logic will not affect the user in any way. For the forms a major update will be is the users needs to make any migrations or some corrections in the filters. So I would say that we are on with staying on the 2+ version |
* update on few small issues
* adding filer and shortcode type
# Conflicts: # CHANGELOG.md # composer.lock # eightshift-forms.php # src/Integrations/Hubspot/HubspotClient.php
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.
Nice work 👏🏼
Left a few suggestions
"infinum/eightshift-coding-standards": "^1.6", | ||
"dealerdirect/phpcodesniffer-composer-installer": "^v1.0.0", | ||
"infinum/eightshift-coding-standards": "2.0.0-beta", | ||
"php-parallel-lint/php-parallel-lint": "^1.3", |
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.
Should we hold this off until the issue with non-escaped folder names has been resolved?
php-parallel-lint/PHP-Parallel-Lint#171 (comment)
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.
I will check what I can do on my side, but we will need to address this definitely :/
@@ -293,6 +293,9 @@ export class Utils { | |||
messageContainer.innerHTML = `<div><span>${msg}</span></div>`; | |||
} | |||
} else { | |||
messageContainer?.classList?.add(this.state.getStateSelector('isActive')); | |||
messageContainer.dataset.status = status; |
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.
messageContainer.dataset.status = status; | |
messageContainer?.dataset.status = status; |
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.
Some minor comments.
I think this update should warrant a major version, as a lot of internal things changed.
"infinum/eightshift-coding-standards": "^1.6", | ||
"dealerdirect/phpcodesniffer-composer-installer": "^v1.0.0", | ||
"infinum/eightshift-coding-standards": "2.0.0-beta", | ||
"php-parallel-lint/php-parallel-lint": "^1.3", |
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.
I will check what I can do on my side, but we will need to address this definitely :/
eightshift-forms.php
Outdated
if (\file_exists(__DIR__ . '/vendor-prefixed/autoload.php')) { | ||
require __DIR__ . '/vendor-prefixed/autoload.php'; | ||
} |
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.
Should a check for the existence of this file also be checked against and returned early if it's missing like for the regular autoloader?
Co-authored-by: Ivan Ramljak <[email protected]>
Co-authored-by: Ivan Ramljak <[email protected]>
Co-authored-by: Ivan Ramljak <[email protected]>
Co-authored-by: Denis Žoljom <[email protected]>
Co-authored-by: Denis Žoljom <[email protected]>
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.
+ other comments
className='es-no-field-spacing' | ||
/> | ||
} | ||
<IconToggle |
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.
It's just Toggle
now (won't fail tho)
Updated
Eightshift-forms-utils
to the latest version2.0.0
.@infinum/eightshift-libs
to the latest version8.0.0
.@infinum/eightshift-frontend-libs
to the latest version12.0.0
.Removed
Data
are not loaded from utils lib.src/Exception/MissingFilterInfoException.php
because it is not used anymore.Added
Caching
service for manifest data.Fixed
Changed
script_dependency_theme
is nowscript_dependency_theme_captcha
.TODO