Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Repeater Control #298

Merged
merged 163 commits into from Aug 22, 2019
Merged

Repeater Control #298

merged 163 commits into from Aug 22, 2019

Conversation

lukecarbis
Copy link
Member

@lukecarbis lukecarbis commented Apr 28, 2019

Closes #36.
Closes #391.

@lukecarbis
Copy link
Member Author

This is still VERY early – it only includes 1 thing: An admin UI for adding / managing Repeater fields.

The "Add Sub-Field" button doesn't even work yet.

But I wanted to get an early review before heading too far down this path to make sure we're happy with the UX. What are your thoughts?

first-pass-at-repeater

@RobStino
Copy link
Collaborator

Hey @lukecarbis,
So excited to see this branch kicked off!!!!!
Here are the designs I mocked up for how I think this could look.
I'm not 100% committed to particular design bits and pieces, but I think it could be important to have the parent field persistently open and "wrapping" the sub fields. I recall we talked about borrowing from the (non-Customizer) menu builder for submenu items, but the above makes the sub-fields feel a bit disconnected from the parent field.
What do you think?
Block_lab_Repeater_Field

@RobStino
Copy link
Collaborator

RobStino commented Apr 28, 2019

Here is a mock of what the frontend could look like as well: https://user-images.githubusercontent.com/1578148/56863483-05b50280-69fa-11e9-9e28-cad86f719af2.png

@lukecarbis
Copy link
Member Author

lukecarbis commented Apr 28, 2019

I actually disagree. I think the backend gets far too crowded like that. Since the headers are all the same, isn't it better to keep all the fields visible without clicking into the repeater?

@lukecarbis
Copy link
Member Author

Added a commit which enables new Sub Fields to be added. There are still a few known issues (e.g. background colours when dragging, dragging sub-rows doesn't work, the auto-scroll doesn't go to the right place), but I just wanted to give you a little more of an idea on how this could work.

@kienstra
Copy link
Collaborator

Looks Good

Hi @lukecarbis,
Nice, this looks good:

adding-sub-field

@RobStino
Copy link
Collaborator

Short screencap of initial thoughts on the WIP.
As discussed in today's team call.
https://d.pr/v/8N9Nrx

@lukecarbis
Copy link
Member Author

@kienstra I merged #221 into this PR, since I know that it's got some changes that affect the repeater field.

Progress so far: The UI for adding a repeater field should now be done. I still haven't gotten around to saving it, and I need to do a general tidy up of the CSS. But those should be the last things to do for the block editor side of things (still lots to do in the editor editor).

@RobStino – could you please review the design / UX so far?

2019-05-29 23-59-45 2019-05-30 00_20_40

lukecarbis and others added 6 commits August 22, 2019 08:18
Before, in /wp-admin > Block Lab > Edit Block,
it wasn't possible to save non-repeater fields.
On clicking 'Update', any non-repeater fields
didn't save.
It looks like this is because $field->settings['parent']
was set, but empty.
So instead, check for it being empty.
It looks like I had broken this,
so it actually displayed the post ID,
instead of the title.
So allow it to show the title.
<p className="bl-dot-tip read-more">
<a href="https://github.com/getblocklab/block-lab/wiki/7.-FAQ" target="_blank">{__( 'Read more', 'block-lab' )}</a>
<a href="https://github.com/getblocklab/block-lab/wiki/7.-FAQ" target="_blank">{ __( 'Read more', 'block-lab' ) }</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@todo: update this link

…ction property

Now, it will simply look at the controls from controls/index.js
This is the previous behavior.
const attr = { ...parentBlockProps.attributes };
const { setAttributes } = parentBlockProps;

if ( undefined !== rowIndex ) { // If this is in a repeater row.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@todo: change order of if/else

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is applied in 5abc564

const attr = { ...parentBlockProps.attributes };

// If the field has a parent, it's probably in a repeater row.
return ( field.parent && attr[ field.parent ][ rowIndex ] ) ? attr[ field.parent ][ rowIndex ][ field.name ] : attr[ field.name ];
Copy link
Collaborator

Choose a reason for hiding this comment

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

@todo: Make this a proper if statement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is applied in a0a0ac2.

kienstra and others added 12 commits August 22, 2019 16:00
As Luke mentioned,
this is easier to understand.
As Luke mentioned,
it'll be easier to have undefined === rowIndex
than undefined !== rowIndex.
This was only used in one place,
so it's not very necessary to have a separate file.
In block_sub_field(),
there was an issue where calling a non-existent

field name produced an error.
There was a workaround that added default values,
but it didn't apply to sub-fields.

Props Luke Carbis.
Before, the repeater attribute was an array.
This will allow more flexibility,
as the repeater can save more values in the future.
@kienstra kienstra merged commit df7ddd9 into develop Aug 22, 2019
@kienstra kienstra deleted the feature/repeater branch August 22, 2019 13:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update "Learn More" links in New Block alert "Repeater" control type
5 participants