-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add sorting to my alerts #212
Conversation
…into kw-add-sorting-to-my-alerts
…into kw-add-sorting-to-my-alerts
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.
Works well, but sorting causes a sideways shift because of the addition of the sort icon. Maybe adding a min-width to the columns would eliminate this shift? It is rather jarring for me.
}; | ||
|
||
MyAlerts.defaultProps = { | ||
reports: [], | ||
alertsSortConfig: { sortBy: 'startDate', direction: 'asc' }, | ||
alertsOffset: 0, | ||
alertsPerPage: 7, |
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.
Should this default to 10?
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 changed it to 10, but right now since the pagination is not working it will be a placeholder only until we find a way to integrate the pagination with sorting (HHS#369)
I adjusted the width - it shouldn't jump now. |
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.
🚀
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.
Lets just hide that pagination text
to={`/activity-reports/${id}`} | ||
href={`/activity-reports/${id}`} | ||
to={idLink} | ||
href={idLink} |
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.
Are both to
and href
required?
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.
Removed href.
} else { | ||
to = offsetTo; | ||
} | ||
return `${from}-${to} of ${reportsCount}`; |
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.
Let's hide this until pagination is active. With 11 reports the table is displaying all 11 (good, since otherwise one would be inaccessible) but this says 1-10 of 11
which is confusing
sequelize, | ||
} from '../models'; | ||
|
||
const orderReportsBy = (sortBy, sortDir) => { |
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.
Nice, I like pulling this out into its own function.
Description of change
Adds sorting to "My Alerts"
How to test
Issue(s)
Checklist