Skip to content

[WIP] Group field type and formatter#40

Closed
damiankloip wants to merge 16 commits into
amitaibu:8.x-1.xfrom
damiankloip:group-formatter
Closed

[WIP] Group field type and formatter#40
damiankloip wants to merge 16 commits into
amitaibu:8.x-1.xfrom
damiankloip:group-formatter

Conversation

@damiankloip

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread src/Og.php Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This method name is a bit confusing. Since this returns an array and Fields is plural this seems to indicate that it returns multiple (?) group audience fields. Maybe better would be getGroupAudienceFieldName().

Initially I thought it would be more useful to return the full field definition, but I suppose the machine name and label are the only interesting properties on it anyway.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This does return multiple? You can have more than one audience field attached to a bundle.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@pfrenssen you can indeed have multiple group audience fields. This usually makes more sense when thinking of the user entity. For example simple_ref would reference a group using the default OG membership type; and premium_ref would reference using some Premium OG membership type.

Each membership created also registers through which field it was saved - so on edit we know how to build the options list.

@damiankloip damiankloip force-pushed the group-formatter branch 3 times, most recently from 41e1564 to fae8557 Compare November 30, 2015 11:02

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Don't think I really like this approach. Was just because it is in theory most flexible for the future. Not sure we would ever need to change this though? If not, I'll just hardcode the value to TRUE again in the OgGroupBoolean class.

@damiankloip

Copy link
Copy Markdown
Collaborator Author

@amitaibu So all the functions that need to be converted in here now, should I put those in separate PRs? Like og_is_member and friends?

@amitaibu

Copy link
Copy Markdown
Owner

Like og_is_member and friends?

Would be nice. Those smaller PRs are nicer to grok, and are removing blocks from other PRs.

@damiankloip

Copy link
Copy Markdown
Collaborator Author

👍

@damiankloip

Copy link
Copy Markdown
Collaborator Author

#53 is providing the isMember method.

@damiankloip

Copy link
Copy Markdown
Collaborator Author

Another dep resolved: #56

@damiankloip

Copy link
Copy Markdown
Collaborator Author

Will tackle og_get_best_group_audience_field next.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Isn't that an extra line?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Eagle eye

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:)

@damiankloip

Copy link
Copy Markdown
Collaborator Author

For adding the subscribe/unsubscribe links: #59 Actually quite a bit of work involved there, as we need the routes, access checking, controller callbacks, and confirmation forms.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@pfrenssen could really do with that shiny new og_get_best_group_audience_field method now :)

@amitaibu

Copy link
Copy Markdown
Owner

@damiankloip will you have time to re-pick this issue, or do you prefer me to take this one?

@amitaibu

amitaibu commented Aug 4, 2016

Copy link
Copy Markdown
Owner

I'll pick this one next, as it compliments #286

@amitaibu

amitaibu commented Aug 4, 2016

Copy link
Copy Markdown
Owner

Closed in favor of #286

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.

4 participants