-
Notifications
You must be signed in to change notification settings - Fork 9
Empty Regions #134
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
Open
lucasconstantino
wants to merge
18
commits into
recidive:master
Choose a base branch
from
TallerWebSolutions:fs/empty-regions
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Empty Regions #134
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
b72bbe1
Created hook responseAlter to allow changing a response right before …
437a4e1
Fixed wrong variable usage.
323ec4a
Merge branch 'fs/response-alter' into fs/empty-regions
98d7126
Altered layout responses to send emptneess information about regions.
414840b
Handle regions that are both panel-region and wrappers.
7cab975
Updated templates to be aware of empty regions.
6a606e7
Updated panel.responseAlter hook usage.
55f0d19
Allow for a region to always be visible.
5b7d903
Minor change to roll back code to what it looked before.
63e8a5e
Renamed hook from responseAlter to response.
25c6128
Changed variable name from alwaysVisible to alwaysRender, to avoid co…
77e02b1
Added alwaysRender field to the regions.
28eaf53
Merge branch 'fs/response-alter' into fs/empty-regions
a789b4b
Fixed wrong validation of alwaysRender option.
35c13e4
Modified name to match new response hook name.
4608e4c
Introducing response() hook to provide a last opportunity for extensi…
recidive 781db77
Merge branch 'master' into fs/response-alter
e4e9432
Merge branch 'fs/response-alter' into fs/empty-regions
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 2 additions & 2 deletions
4
applications/default/extensions/layout/public/templates/column.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| <div ng-if="column.region" ng-include="'/templates/region.html'" ng-controller="RegionController"></div> | ||
| <div ng-if="column.region && !column.empty" ng-include="'/templates/region.html'" ng-controller="RegionController"></div> | ||
| <!-- Rows --> | ||
| <div ng-repeat="row in column.rows | orderBy:'weight'" ng-controller="RowController" ng-include="getTemplate()" class="row-{{row.name}} row-fluid"></div> | ||
| <div ng-repeat="row in column.rows | orderBy:'weight'" ng-if="!row.empty" ng-controller="RowController" ng-include="getTemplate()" class="row-{{row.name}} row-fluid"></div> |
2 changes: 1 addition & 1 deletion
2
applications/default/extensions/layout/public/templates/layout.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,2 @@ | ||
| <!-- Rows --> | ||
| <div ng-repeat="row in layout.rows | orderBy:'weight'" ng-controller="RowController" ng-include="row.template || '/templates/row.html'" class="row-{{row.name}} row-fluid"></div> | ||
| <div ng-repeat="row in layout.rows | orderBy:'weight'" ng-if="!row.empty" ng-controller="RowController" ng-include="row.template || '/templates/row.html'" class="row-{{row.name}} row-fluid"></div> |
4 changes: 2 additions & 2 deletions
4
applications/default/extensions/layout/public/templates/row.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| <div ng-class="row.classes" class="container"> | ||
| <div ng-if="row.region" ng-include="'/templates/region.html'" ng-controller="RegionController"></div> | ||
| <div ng-if="row.region && !row.empty" ng-include="'/templates/region.html'" ng-controller="RegionController"></div> | ||
| <!-- Columns --> | ||
| <div ng-repeat="column in row.columns | orderBy:'weight'" ng-controller="ColumnController" ng-include="getTemplate()" ng-class="column.classes" class="column-{{column.name}} col-md-{{column.width}}"></div> | ||
| <div ng-repeat="column in row.columns | orderBy:'weight'" ng-if="!column.empty" ng-controller="ColumnController" ng-include="getTemplate()" ng-class="column.classes" class="column-{{column.name}} col-md-{{column.width}}"></div> | ||
| </div> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,19 +234,24 @@ panel.contextReactionType = function(reactionTypes, callback) { | |
| } | ||
| }, | ||
| react: function(request, response, regionPanels, callback) { | ||
|
|
||
| // Get panel type. | ||
| var Panel = self.application.type('panel'); | ||
|
|
||
| // Initialize panels container if not initialized yet. | ||
| response.payload.panels = response.payload.panels || {}; | ||
|
|
||
| // Add all panels to response payload. | ||
| async.each(Object.keys(regionPanels), function(regionName, next) { | ||
|
|
||
| // Initialize panels container if not initialized yet. | ||
| response.payload.panels[regionName] = response.payload.panels[regionName] || []; | ||
| var panels = regionPanels[regionName]; | ||
|
|
||
| // A context can set several panels for a region. | ||
| async.each(panels, function(regionPanel, next) { | ||
| // Load the panel. | ||
| var Panel = self.application.type('panel'); | ||
|
|
||
| // Load the panel. | ||
| Panel.load(regionPanel.name, function(err, panel) { | ||
| if (err) { | ||
| return callback(err); | ||
|
|
@@ -274,3 +279,45 @@ panel.contextReactionType = function(reactionTypes, callback) { | |
|
|
||
| callback(null, newReactionTypes); | ||
| }; | ||
|
|
||
| /** | ||
| * The response() hook. | ||
| */ | ||
| panel.response = function (payload, request, response, callback) { | ||
|
|
||
| var layout = payload.data && payload.data.layout; | ||
| var panels = payload.data && payload.data.panels; | ||
|
|
||
| if (layout && panels) { | ||
|
|
||
| /** | ||
| * Recursively verifies if a row/column is empty. | ||
| */ | ||
| function findContent(region, childType) { | ||
|
|
||
| // Consider empty by default, unless when region is told to always be | ||
| // rendered. | ||
| region.empty = region.alwaysRender ? false : true; | ||
|
|
||
| // Check if region has content. | ||
| if (region.empty && region.region == true) { | ||
| region.empty = !Boolean(panels[region.name] && panels[region.name].length); | ||
| } | ||
|
|
||
| // Iterate recursively | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing a period on this comment. |
||
| if (region[childType] && region[childType].length) { | ||
| region[childType].forEach(function (childRegion) { | ||
| var childEmpty = findContent(childRegion, childType == 'rows' ? 'columns' : 'rows'); | ||
| region.empty = !region.empty ? region.empty : childEmpty; | ||
| }); | ||
| } | ||
|
|
||
| return region.empty; | ||
| } | ||
|
|
||
| // As a layout contains rows, it behaves much like a column itself. | ||
| findContent(layout, 'rows'); | ||
| } | ||
|
|
||
| callback(); | ||
| }; | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This looks like a good place to use async.detect() or some other asynchronous utility from async library.
Could you justify there would be no impact in performance by doing those blocking loops? Specially since this will run in all page requests.
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.
Although async does not help if recursiveness (or am I wrong?), indeed it could be used in the children iteration and would in fact improve performance for a bit. Anyway, I don't think there would be any issue on performance because of this loop, as it is really not probable to have a deep layout structure and as the
forEachcode is run natively and inside that resides a quite simple validation. I would run some tests, though, but I'm sure they would result in nothing to worry about.It is good to note that the "heavy part" is only runned when receiving actual layout & panels data on payload - which as I've tested does not correspond to many request situations.
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.
Ok, let's do that this way for now, and review after we have some benchmarks.