Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

fix(home): Update Home dashboard with New Designs #2648

Merged
merged 2 commits into from
Mar 23, 2018
Merged

fix(home): Update Home dashboard with New Designs #2648

merged 2 commits into from
Mar 23, 2018

Conversation

AdamJ
Copy link
Collaborator

@AdamJ AdamJ commented Mar 19, 2018 •
edited
Loading

This PR is to update the Home Dashboard HTML/CSS with the new designs.

@AdamJ
Copy link
Collaborator Author

AdamJ commented Mar 20, 2018

A PR to my branch helped to clean up some options and have it fall more inline with the Feature Flag work.
From @corinnekrych and @xcoulon:

  • newHomeDashboard is marked internal, therefore wrapper (f8-feature-toggle component) will display the user-level section if user-enabled is true otherwise it will display the default-level
  • to deal with the localhost I've added an attribute restricted with default value. Once ready to be shown on prod-preview, just remove restricted=dev

@AdamJ AdamJ force-pushed the home-dashboard branch 2 times, most recently from e6d8c24 to e6a0a7e Compare March 20, 2018 19:07
@AdamJ
Copy link
Collaborator Author

AdamJ commented Mar 20, 2018

@corinnekrych I've updated the rest of the components that the Home Dashboard uses to utilize the new flagging method that you and @xcoulon came up with. Please take a look and let me know how it works for you :)

@AdamJ AdamJ mentioned this pull request Mar 20, 2018
Closed
2 tasks
dlabrecq
dlabrecq suggested changes Mar 21, 2018
View reviewed changes
Copy link
Collaborator

@dlabrecq dlabrecq left a comment

Choose a reason for hiding this comment

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

I was able to run locally without changing the developmentEnabled properties as described.

src/app/dashboard-widgets/recent-pipelines-widget/recent-pipelines-widget.component.ts Outdated
@@ -17,11 +20,19 @@ export class RecentPipelinesWidgetComponent implements OnInit {
contextPath: Observable<string>;
buildConfigs: ConnectableObservable<BuildConfigs>;
buildConfigsCount: Observable<number>;
newHomeDashboardEnabled: boolean = false;
developmentEnabled: boolean = false; // set to false to hide for prod - set to true for local development
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see developmentEnabled used anywhere in the component or HTML template?

src/app/home/home.component.ts Outdated
developmentEnabled: boolean = false; // set to false to hide for prod - set to true for local development
myInterval: number = 0;
noWrapSlides: boolean = true;
showIndicator: boolean = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

showIndicator and developmentEnabled appear to be unused.

src/app/home/home.component.ts Outdated
newHomeDashboardEnabled: boolean = false;
developmentEnabled: boolean = false; // set to false to hide for prod - set to true for local development
myInterval: number = 0;
noWrapSlides: boolean = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably set the myInterval and noWrapSlides properties on the carousel directly, in the HTML template, as these are not likely to change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using the methods documented on the ngx-bootstrap site: https://valor-software.com/ngx-bootstrap/#/carousel. They have it broken out into template and component, even for a single item.

src/app/home/home.component.ts Outdated
@@ -52,6 +61,10 @@ export class HomeComponent implements OnInit, OnDestroy {
this.space = '';
this.selectedFlow = 'start';
this._spaceSubscription = spaces.recent.subscribe(val => this.recent = val);
this.subscriptions.push(featureTogglesService.getFeature('newHomeDashboard').subscribe((feature) => {
this.newHomeDashboardEnabled = feature.attributes['enabled'] && feature.attributes['user-enabled'];
console.log('~', this.newHomeDashboardEnabled);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should remove the log output

@AdamJ
Copy link
Collaborator Author

AdamJ commented Mar 21, 2018

@dlabrecq fixed the issues you commented on and removed unnecessary code. I made a not on the ngx-bootstrap implementation of the carousel.

@fabric8cd
Copy link
Contributor

@mindreeper2420 fabric8/fabric8-ui:SNAPSHOT-PR-2648-3 fabric8-ui is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2648-fabric8-ui.badger.fabric8.io

dlabrecq
dlabrecq reviewed Mar 21, 2018
View reviewed changes
src/app/home/home.component.ts Outdated
@Component({
encapsulation: ViewEncapsulation.None,
selector: 'alm-home',
templateUrl: './home.component.html',
styleUrls: ['./home.component.less']
})
export class HomeComponent implements OnInit, OnDestroy {
newHomeDashboardEnabled: boolean = false;
myInterval: number = 0;
noWrapSlides: boolean = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing wrong with following the example. However, the carousel can be simplified to just this:

<carousel interval="0" noWrap="true">

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all cleaned up

@fabric8cd
Copy link
Contributor

@mindreeper2420 fabric8/fabric8-ui:SNAPSHOT-PR-2648-4 fabric8-ui is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2648-fabric8-ui.badger.fabric8.io

@fabric8cd
Copy link
Contributor

@mindreeper2420 fabric8/fabric8-ui:SNAPSHOT-PR-2648-5 fabric8-ui is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2648-fabric8-ui.badger.fabric8.io

@joshuawilson
Copy link
Collaborator

@mindreeper2420 is this ready for review? I tried the badger link with Internal set and don't see the new dashboard.

@AdamJ
Copy link
Collaborator Author

AdamJ commented Mar 22, 2018

@joshuawilson it is ready for review, but you have to do so locally. There is an additional flag set restricted=dev that prevents it from being seen unless run locally.

@joshuawilson
Copy link
Collaborator

Ah, so you will never see this in prod no matter what setting you have.

@AdamJ
Copy link
Collaborator Author

AdamJ commented Mar 23, 2018

Correct - the flags will be removed once all development is completed.

@joshuawilson joshuawilson merged commit bf70124 into fabric8-ui:master Mar 23, 2018
@AdamJ AdamJ mentioned this pull request Mar 23, 2018
Closed
2 tasks
@catrobson catrobson mentioned this pull request Apr 4, 2018
Closed
2 tasks
fabric8cd pushed a commit that referenced this pull request Dec 16, 2018
@vikram-raj @nimishamukherjee
fix(iteration): Allow user to select past and future start date. (#2648)
d554eeb 
* fix(iteration): Allow user to select past and future start date.

* fix(iteration): disable year selector.

* fix(test): fix test selector.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dlabrecq dlabrecq dlabrecq approved these changes

@dgutride dgutride Awaiting requested review from dgutride

@corinnekrych corinnekrych Awaiting requested review from corinnekrych

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AdamJ @fabric8cd @joshuawilson @dlabrecq