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

group components on manage subscriptions page #1983

Merged
merged 6 commits into from
Jul 18, 2016

Conversation

peelman
Copy link
Contributor

@peelman peelman commented Jul 18, 2016

A possible solution to #1909

@@ -0,0 +1,18 @@
<li class="list-group-item {{ $component->group_id ? "sub-component" : "component" }}">
<div class="checkbox">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please correct this indentation.

@GrahamCampbell GrahamCampbell added this to the V2.4.0 milestone Jul 18, 2016
*/
public function has_subscriber($subscriptions)
{
$enabled_components = $this->wrappedObject->enabled_components()->orderBy('order')->pluck('id')->toArray();
Copy link
Member

Choose a reason for hiding this comment

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

Please correct the indentation here too.

@peelman
Copy link
Contributor Author

peelman commented Jul 18, 2016

I'm going through the entire thing now; it was done on-server using Nano, so the indentation is all whacked up. Going through with TextMate now and cleaning those files up.

@endforeach
<div class="panel-body">
@if(!$component_groups->isEmpty() || !$ungrouped_components->isEmpty())
@include('partials.components_form')
Copy link
Member

Choose a reason for hiding this comment

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

Please don't indent the Blade syntax 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you clarify here? What's broken with the indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying to left-align the @includes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please :)

@jbrooksuk
Copy link
Member

Awesome, thanks!

@jbrooksuk jbrooksuk self-assigned this Jul 18, 2016
@peelman
Copy link
Contributor Author

peelman commented Jul 18, 2016

open to any other feedback gents; happy to tweak before any merges happen.

@@ -147,15 +148,19 @@ public function showManage($code = null)
}

$subscriber = Subscriber::where('verify_code', '=', $code)->first();
$usedComponentGroups = Component::enabled()->where('group_id', '>', 0)->groupBy('group_id')->pluck('group_id');
Copy link
Member

Choose a reason for hiding this comment

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

@CachetHQ/owners I feel like we need a better way to handle this and the next 2 lines too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have only dealt with laravel briefly in the past; so I apologize in advance for anything that is overly hack-ish :)

Copy link
Member

Choose a reason for hiding this comment

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

Not at all, this is something for us to deal with, you've done the right thing.

@@ -13,7 +13,7 @@
</li>
<div class="form-group group-items {{ $componentGroup->has_subscriber($subscriptions) ? null : "hide" }}">
@foreach($componentGroup->enabled_components()->orderBy('order')->get() as $component)
@include('partials.component_input', compact($component))
@include('partials.component_input', compact($component))
Copy link
Member

Choose a reason for hiding this comment

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

Not that far left, sorry. Just so that it's directly underneath the @foreach or @if etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@jbrooksuk jbrooksuk merged commit bcbbee2 into cachethq:master Jul 18, 2016
@peelman peelman deleted the improve-manage-subscriptions branch July 18, 2016 14:06
@joecohens
Copy link
Contributor

👍

@peelman peelman restored the improve-manage-subscriptions branch July 19, 2016 13:50
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.

None yet

4 participants