Feature/feedback #35
Feature/feedback #35
Conversation
| this.open = !this.open; | ||
| let mainDiv = this.el.nativeElement.querySelector('#main'); | ||
| if (this.open) { | ||
| mainDiv.style['width'] = '25%'; |
There was a problem hiding this comment.
You might consider using ngClass to apply a class with width: 25%
| name:string = ''; | ||
| comment: string = ''; | ||
| statusMessage:string = ''; | ||
| error = false; |
There was a problem hiding this comment.
Please rename to hasError
|
|
||
| buttonClicked(){ | ||
| this.open = !this.open; | ||
| let mainDiv = this.el.nativeElement.querySelector('#main'); |
| }) | ||
| export class FeedbackWidgetComponent implements OnInit { | ||
| open = false; | ||
| email: string = ''; |
There was a problem hiding this comment.
You can rely on implicit types if you are initializing these strings to empty string.
Why are you initializing them to empty string?
| export class FeedbackWidgetComponent implements OnInit { | ||
| open = false; | ||
| email: string = ''; | ||
| name:string = ''; |
| styleUrls: ['./feedback-widget.component.css'] | ||
| }) | ||
| export class FeedbackWidgetComponent implements OnInit { | ||
| open = false; |
There was a problem hiding this comment.
Please rename to isOpen
| timestamp: new Date().toUTCString(), | ||
| href:window.location.href, | ||
| header: headerText | ||
| }).then(x => { |
| header: headerText | ||
| }).then(x => { | ||
| this.statusMessage = 'Successfully sent'; | ||
| setTimeout(() => { |
There was a problem hiding this comment.
This send function is doing a lot of stuff. I recommend refactoring this message display logic into a private function.
Then you can call
}).then(this.showSuccessMessage);| } | ||
| } | ||
|
|
||
| htmlEscape(str) { |
There was a problem hiding this comment.
Can you add a function level comment to describe what this is doing? Alternatively, is there a npm module you could use? It feels like someone has done this before, somewhere.
| let comment = this.comment; | ||
| let email = this.email; | ||
| let items = this.angularFire.database.list('/feedback'); | ||
| let headerText = document.body.querySelector('h1:not([style*="display:none"]') ? document.body.querySelector('h1:not([style*="display:none"]').innerHTML : ''; |
|
|
||
| buttonClicked(){ | ||
| this.open = !this.open; | ||
| let mainDiv = this.el.nativeElement.querySelector('#main'); |
| }); | ||
|
|
||
| this.comment = ''; | ||
| //TODO set username in state.local.user |
There was a problem hiding this comment.
Is it a real TODO or copyPaste?
| } | ||
| } | ||
|
|
||
| htmlEscape(str) { |
There was a problem hiding this comment.
Don't think this is super secure, leave a TODO, since we'll have to figure out how to handle it once we fetch it
- Shows all slides in a single scrollable view - Added enumeration of "Presentation modes", could be "none" or "overview" - Toggles between 'overview' and 'none' - Added @print media-query to print 1 slide per page and use proper zoom - A slide shows its page number if in 'overview' mode - Added Overview button to all slides
- Shows all slides in a single scrollable view - Added enumeration of "Presentation modes", could be "none" or "overview" - Toggles between 'overview' and 'none' - Added @print media-query to print 1 slide per page and use proper zoom - A slide shows its page number if in 'overview' mode - Added Overview button to all slides
|
OK, I need this in ASAP for the demo, please fix this stuff in another PR |
|
In the future, can you create a demo branch and merge the branch into that without closing the PR? It will be difficult to respond to review over different pull requests. |
|
Agreed |
This PR adds a feedback form which sends feedback to firebase