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
feat: added preferences to toggle on/off sidebar #48
feat: added preferences to toggle on/off sidebar #48
Conversation
src/app/page/page.component.ts
Outdated
@@ -24,7 +24,12 @@ namespace Page { | |||
this.previousWidth = this.$window.innerWidth; | |||
}); | |||
}); | |||
this.$rootScope.$on(CLOSE_MAIN_NAV_EVENT, () => this.isNavOpen = false); | |||
var curr_state = window.localStorage.getItem('current_state'); | |||
if (curr_state === "ON") |
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.
Always use {
and }
for if
. I don't think we omit these in other places.
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.
If we use Enum for the values it can be switch
statement.
src/app/preferences/general-preferences/general-preferences.controller.ts
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.html
Outdated
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.module.ts
Outdated
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
I have incorporated all the requested changes in the commit aa344f9. Please check it and provide further feedback :) |
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
return this.$window.localStorage.getItem('defaultVerticalNavState'); | ||
|
||
} | ||
setDefaultVerticalnavstate(defaultVerticalNavState): void { |
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.
same here
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.
and what about using enum for the state variable?
src/app/preferences/general-preferences/general-preferences.module.ts
Outdated
Show resolved
Hide resolved
src/app/page/page.component.ts
Outdated
@@ -24,7 +24,14 @@ namespace Page { | |||
this.previousWidth = this.$window.innerWidth; | |||
}); | |||
}); | |||
this.$rootScope.$on(CLOSE_MAIN_NAV_EVENT, () => this.isNavOpen = false); | |||
let curr_state = window.localStorage.getItem('defaultVerticalNavState'); | |||
if (curr_state === "show") { |
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.
pls use enum I suggested
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.
and regarding where the VerticalNavState
enum type should be defined, there are a few options. But I think the simplest is just put it in the page.component.ts
and then from the general-preferences.service.ts
reference it.
And I'd prefer the commits to be only one. Please force push to the pr branch instead of adding a commit on top of the original. |
28e8b44
to
3ae6952
Compare
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
2308b66
to
70b4697
Compare
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.
Final round of change requests.
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
src/app/preferences/general-preferences/general-preferences.service.ts
Outdated
Show resolved
Hide resolved
fb5f2e0
to
c8baaff
Compare
getDefaultVerticalNavState(): string { | ||
if (this.$window.localStorage.getItem('defaultVerticalNavState') !== null) { | ||
return this.$window.localStorage.getItem('defaultVerticalNavState'); | ||
} else |
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.
One more last change:
} else {
...
}
119940f
to
cc56ec0
Compare
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.
Well done!
Implemented a feature which solves the issue:
Issue link: hawtio/hawtio#2657