-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: redesigns sessions page and add lock/unlock column #2964
feat: redesigns sessions page and add lock/unlock column #2964
Conversation
ad01219
to
dedc981
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.
Looks good, push the corresponding server PR soon!
@uds5501 @CosmicCoder96 @pradeepgangwar @kushthedude please review |
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.
Everything looks nice !
@@ -0,0 +1,14 @@ | |||
{{record.title}}<br> |
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.
We generally avoid the use of the br tag. You can use a hidden divider instead.
Also there's no point of using a property name if you are using record.title simply to get the title.
May be you can use (get record column.propertyName) for it.
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.
@ritikamotwani when I use <div class="hidden ui divider"></div>
instead of <br>
the separation becomes too large.
With hidden divider
Should I implement this 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.
@shreyanshdwivedi , use the hidden divider one.
@@ -0,0 +1,9 @@ | |||
{{#if record.isLocked}} | |||
{{#ui-popup content=(t 'Unlock Session') class='ui icon button' click=(action unlockSession record) position='left center'}} |
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.
Make it a basic button like the other action buttons.
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.
@ritikamotwani all the action buttons are also ui-popup
as we need to show tooltip on hovering the buttons displaying the specific task which they carry out
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.
Yes, I know what a popup is. I am talking about the class of the button used. Make it ui basic :)
e9b4320
to
92e41a6
Compare
fixes codacy issues updates buggy unlockSession condition and error message hide edit option for locked sessions fixes the class of lock icon and removes br tag
92e41a6
to
2fc76a2
Compare
@ritikamotwani I've updated the PR. Please review |
@@ -72,6 +77,36 @@ export default Controller.extend({ | |||
viewSession(id) { | |||
this.transitionToRoute('events.view.sessions.edit', id); | |||
}, | |||
lockSession(session) { | |||
session.set('isLocked', true); | |||
this.set('isLoading', true); |
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.
Everything looks good, but just had a question, wouldn't it be nice if the session.set('isLocked', true)
be executed when isLoading is true? (I think it's just code design, won't affect the functionality, of course)
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.
@uds5501 I did this intentionally. I had a number of references where values were set before setting isLoading
to true. As we know that isLoading
is basically for loading status if a task takes time so it is added just before the time-taking task. However, if you want you can refer to login-form
or register-form
where similar implementations were carried out.
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.
Understood!
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.
LGTM!
@@ -0,0 +1,15 @@ | |||
{{record.title}} |
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.
Also there's no point of using a property name if you are using record.title simply to get the title.
May be you can use (get record column.propertyName) for it.
this.send('refreshRoute'); | ||
}) | ||
.catch(() => { | ||
this.notify.error(this.get('l10n').t('An unexpected error has occurred.')); |
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.
Use this.notify....this.l10n
95ec7de
@CosmicCoder96 @pradeepgangwar please review |
Fixes #2958
Short description of what this resolves:
Redesigns the session page and introduce lock/unlock column
Changes proposed in this pull request:
If session is locked
Checklist
development
branch.