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

new layout (separable palette) tidies #93

Merged
merged 9 commits into from Oct 30, 2018

Conversation

@ahgittin
Copy link
Contributor

commented Oct 25, 2018

#84 introduces the separable palette/config panel

this enhances that so that the palette gets the right size, and also fixes extension points and makes it so the "add (member spec)" usage of the panel respects those extension points

minor tidies to dropdowns and other style points also

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

retest this please

@tbouron
Copy link
Member

left a comment

Thanks for this @ahgittin, I have a couple of comments but they are pretty minor.

@@ -30,15 +37,44 @@ export function catalogSelectorDirective() {
scope: {
family: '<',
onSelect: '&',
itemsPerPage: '<',
rowsPerPage: '<', // if unset then fill

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

If this parameter is optional, you should add a ?

<button class="btn btn-info btn-outline" ng-click="activateModal()">{{buttonText}}</button>

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

Ahaha my bad on this one :)

const ITEMS_PER_PAGE = 30;
const MIN_ROWS_PER_PAGE = 4;

const PALETTE_VIEW_MODES = {

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

What about adding an extension point here to customise this object? Would be useful

This comment has been minimized.

Copy link
@ahgittin

ahgittin Oct 30, 2018

Author Contributor

not convinced. could be changed when useful.


// repaginate when load completes (and items are shown), or it is resized
$scope.$watchGroup(
[ () => $scope.isLoading, () => main[0].offsetHeight, () => $scope.state.viewMode.itemsPerRow ],

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

I don't think you need to watch the container height. We resize the windows because we are Devs but users won't do that. It doesn't do any harm though

if (!rowsPerPage) {
let main = angular.element($element[0].querySelector(".catalog-palette-main"));
if (!main || main[0].offsetHeight==0) {
// console.log("no main or hidden or items per page fixed");

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

To remove

$scope.isLoading = true;

$scope.$watch('search', () => {
$scope.freeFormTile = {
symbolicName: $scope.search,
name: $scope.search,
displayName: $scope.search,
supertypes: [$scope.family.superType]
supertypes: ($scope.family ? [ $scope.family.superType ] : []),

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

$scope.family is marked as required so this should not be needed. Callers of the catalog-selector directive should always pass it

This comment has been minimized.

Copy link
@ahgittin

ahgittin Oct 30, 2018

Author Contributor

can be passed in as null however

delete sections[id];
return old;

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

I'm curious, why returning old?

This comment has been minimized.

Copy link
@ahgittin

ahgittin Oct 30, 2018

Author Contributor

if you want to change order a common pattern would be to delete it but put it to a local var so you could then add it in the right place. same idea as Map.remove

@@ -51,8 +52,11 @@ export function GraphicalEditAddController($scope, $filter, $state, $stateParams
break;
}

$scope.catalogItemsPerPage = 24;

if (!$scope.familiesToShow) $scope.familiesToShow = [ $scope.family ];

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

Could you use curly brackets for the if statement please? That's to be consistent with the rest of the codebase and also make it easier to read

switch ($stateParams.family) {
case EntityFamily.ENTITY.id.toLowerCase():
$scope.family = EntityFamily.ENTITY;
break;
case EntityFamily.SPEC.id.toLowerCase():
$scope.family = EntityFamily.SPEC;
$scope.familiesToShow = [ EntityFamily.ENTITY, EntityFamily.SPEC ];

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

Seems to be used only in the JS code so would be better to be a local var (i.e. avoiding adding stuff to the $scope as a general rule of thumb)

This comment has been minimized.

Copy link
@ahgittin

ahgittin Oct 30, 2018

Author Contributor

no, it's used in the html

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

I cannot find any reference if this on templates

@@ -243,4 +285,6 @@ function controller($scope, $element, $q, $uibModal, $log, $templateCache, palet

// allow downstream to configure this controller and/or scope
(composerOverrides.configurePaletteController || function() {})(this, $scope, $element);

$scope.paletteItemClasses = "col-xs-3";

This comment has been minimized.

Copy link
@tbouron

tbouron Oct 30, 2018

Member

This doesn't seems to be used so can be removed

This comment has been minimized.

Copy link
@ahgittin

ahgittin Oct 30, 2018

Author Contributor

yes, removed in #94

ahgittin added a commit to ahgittin/brooklyn-ui that referenced this pull request Oct 30, 2018
@ahgittin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

all addressed as part of #94

@asfgit asfgit merged commit 046c656 into apache:master Oct 30, 2018

1 check passed

Jenkins: brooklyn-ui-pull-request SUCCESS 10 tests run, 0 skipped, 0 failed.
Details
asfgit pushed a commit that referenced this pull request Oct 30, 2018
@tbouron

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Thanks @ahgittin, merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.