-
Notifications
You must be signed in to change notification settings - Fork 99
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
Projects in the sidebar #414
Conversation
7254b39
to
8230a81
Compare
@@ -11,15 +11,15 @@ | |||
|
|||
{{#unless @isCollapsed}} | |||
<div | |||
class="sidebar-body scrollable-container" |
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.
Unused class
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.
Couple questions, but nothing blocking!
/** | ||
* The action to show the Projects modal. | ||
* Triggered by clicking the "+" button in the Projects section. | ||
*/ | ||
@action protected showProjectsModal() { | ||
this.projectsModalIsShown = true; | ||
} | ||
|
||
/** | ||
* The action to hide the Projects modal. | ||
* Passed to the Projects modal component and | ||
* triggered on modal close. | ||
*/ | ||
@action protected hideProjectsModal() { | ||
this.projectsModalIsShown = false; | ||
} | ||
|
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.
Non-blocking: maybe could simplify to a toggleProjectsModall()
that does this.projectsModalIsShown = !this.projectsModalIsShown
?
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.
I usually reserve the toggle
construction for actual toggles, e.g., "Show more/Show fewer" or check/uncheck controls. Here, because the show and hide actions are separate, I like the clarity of being specific:
Using <button {{on "click" this.showProjectsModal}}>
and <Hds::Modal @onClose={{this.hideProjectsModal}}>
feels more precise to me than if we were to pass toggleProjectsModal
to both.
Also, it's not the case here, but show/hide actions often have specific setup or teardown, so I might also be in the habit of separating them from the jump.
@@ -107,6 +122,31 @@ export default class NewProjectFormComponent extends Component<NewProjectFormCom | |||
}), | |||
}) | |||
.then((response) => response?.json()); | |||
|
|||
/** | |||
* Create the project with a minimum duration. |
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.
2 fast 2 furious
* Will exist if the issue is a full Jira issue. | ||
* Determines if the type is shown in the widget. | ||
*/ | ||
protected get issueType() { |
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.
Curious why we need these getters and can't just do conditionals in the template based on if the issue's properties are undefined or not?
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.
For some reason Glint has trouble when using union types like this
{{! this.issue: PickedJiraIssue | JiraIssue }}
{{#if this.issue.issueType}}
{{!-- if this passes, it means this.issue is type: JiraIssue --}}
{{!-- however Glint thinks it still might be a PickedJiraIssue --}}
{{this.issue.issueType}}
{{!-- this gets underlined by Glint --}}
{{/if}}
But the getter works.
Adds a Projects section to the document sidebar along with "add document to project" functionality.