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

Fields redux store #108

Merged
merged 28 commits into from
Jun 11, 2021
Merged

Fields redux store #108

merged 28 commits into from
Jun 11, 2021

Conversation

thomasplevy
Copy link
Contributor

Description

Closes: #103
Closes: #104
Closes: https://github.com/orgs/gocodebox/projects/12#card-62005666

How has this been tested?

Manually, and it needs more manual testing
E2E tests to be written

Screenshots

Types of changes

Bug fixes & new features

Checklist:

  • My code has been tested.
  • My code passes all existing automated tests.
  • My code follows the LifterLMS Coding & Documentation Standards.

@thomasplevy
Copy link
Contributor Author

@eri-trabiccolo mostly requesting a review to help test and make sure nothings gone haywire here.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Scan Summary

Tool Critical High Medium Low Status
Dependency Scan (nodejs) 3 20 14 2
Security Audit for Infrastructure 0 0 0 0
PHP Security Analysis 0 0 0 0
PHP Security Audit 0 0 3 0

Recommendation

Please review the findings from Code scanning alerts before approving this pull request. You can also configure the build rules or add suppressions to customize this bot 👍

Copy link
Contributor

@eri-trabiccolo eri-trabiccolo left a comment

Choose a reason for hiding this comment

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

Ohhh this is a quite big one.
Overall I like it, need to test it a little bit.
E.g. I've found that clearing a field's name because you want to replace the default name with another one is "impossible". The notice The name "" is not unique. Please choose a globally unique field name. appears and
key
name
id
attributes get regenerated.

src/js/blocks/form-fields/inspect.js Outdated Show resolved Hide resolved
src/js/blocks/form-fields/inspect.js Outdated Show resolved Hide resolved
src/js/blocks/form-fields/settings.js Show resolved Hide resolved
src/js/data/fields/reducer.js Outdated Show resolved Hide resolved
@eri-trabiccolo
Copy link
Contributor

I see I can duplicate some llms core reusable blocks, e.g. the phone number. Is this expected?

@eri-trabiccolo
Copy link
Contributor

Also weird thing, looks like the confirmation fields do not display anymore in the editor? [regression]
Plus when you duplicate "confirmation groups" reusable blocks (password and email) both the duplicate and the original name,key,id attributes are altered (so you're asked to save the reusable block) - with the original getting the values of duplicate's... can you reproduce this?

@thomasplevy
Copy link
Contributor Author

I see I can duplicate some llms core reusable blocks, e.g. the phone number. Is this expected?

How are you going about doing this?

Copy link
Contributor

@eri-trabiccolo eri-trabiccolo left a comment

Choose a reason for hiding this comment

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

This changeset pretty good to me, I like the new warning system.

@thomasplevy thomasplevy merged commit d6d2ffb into dev Jun 11, 2021
@thomasplevy thomasplevy deleted the fields-redux-store branch September 29, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants