Skip to content

Hotfix/dynatable i18n text duplication#1018

Merged
jykae merged 17 commits intodevelopfrom
hotfix/dynatable-i18n-text-duplication
Jun 3, 2016
Merged

Hotfix/dynatable i18n text duplication#1018
jykae merged 17 commits intodevelopfrom
hotfix/dynatable-i18n-text-duplication

Conversation

@frenchbread
Copy link
Copy Markdown
Contributor

Closes #785

Changes:

  • Replace dynatable with datatables

Fixes:

  • Language change

@jykae
Copy link
Copy Markdown
Contributor

jykae commented Jun 2, 2016

@frenchbread Reviewing

@jykae
Copy link
Copy Markdown
Contributor

jykae commented Jun 2, 2016

Original issue seems to be solved. However rendering in UI, timestamps, etc. still not very nice in Chrome at least. Should we work it on this PR or what are your thoughts @frenchbread ?

@jykae jykae added the WIP label Jun 2, 2016
@jykae
Copy link
Copy Markdown
Contributor

jykae commented Jun 2, 2016

Decided to work on the CSS layout issues on this PR with @frenchbread
Marked WIP.

@frenchbread
Copy link
Copy Markdown
Contributor Author

@jykae I am using this package here. The thing is that those options for pagination does not work. E.g ${'.datatable').dataTable() - works, $('.datatable').dataTable({"sPaginationType": "bs_two_button"}) - does not, gives an error.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented Jun 2, 2016

@frenchbread the last commit on the Datatables package was Dec 11, 2014 @Jowin. Is there a more active package?

@frenchbread
Copy link
Copy Markdown
Contributor Author

@jykae I've added changes. Please review. The only issue I'm getting now is when formatting unix timestamp it gives wierd year value: e.g: 9/08/48387 11:10:01

@jykae
Copy link
Copy Markdown
Contributor

jykae commented Jun 2, 2016

🆒 checking

@brylie good note about the package health and possible future support. EDIT: Ok, that was addressed already in commit 77ec71d

@frenchbread
Copy link
Copy Markdown
Contributor Author

@brylie @jykae Latest commits contain different package. It's still datatables but official and native one for NPM.

@jykae
Copy link
Copy Markdown
Contributor

jykae commented Jun 2, 2016

Good work so far a bit beyond than the explanation in the PR.
Let's return to this tomorrow, I will check the server time settings and whether Umbrella has any issues related to this. For me it looks like you are doing the conversion anyway right with momentjs.

@frenchbread frenchbread removed the WIP label Jun 3, 2016
@frenchbread
Copy link
Copy Markdown
Contributor Author

Removing WIP. Please review.

@jykae
Copy link
Copy Markdown
Contributor

jykae commented Jun 3, 2016

Checked in the review together, merging. Good work @frenchbread

@jykae jykae merged commit 6a59b5d into develop Jun 3, 2016
@jykae jykae removed the in progress label Jun 3, 2016
@jykae jykae deleted the hotfix/dynatable-i18n-text-duplication branch June 3, 2016 12:02
initDatatable();

// Init datatable function
function initDatatable () {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define this function in the onCreated callback, then call it from within this onRendered callback.

@frenchbread
Copy link
Copy Markdown
Contributor Author

@brylie I think there is no need to move those functions because refreshTable() is wrapped into instance.renderCharts() which is already located in onCreated callback for initialisation. instance.renderCharts() is called from instance.getDashboardData() also in onCreated. But the intance.getDashboardData() itself with the entire functions is called from onRendered. So there should not be any rendering problems.

@brylie
Copy link
Copy Markdown
Contributor

brylie commented Jun 6, 2016

Part of modularizing the code will be to move nested functions to the template level. E.g. our code is hard to maintain and refactor, in part, because functions are defined and called inside of other functions.

@frenchbread
Copy link
Copy Markdown
Contributor Author

@brylie Agreed. Created #1040 for that - feel free to extend def. of done. I'm thinking that we should move more to object-oriented approach there (to be able to easily reuse functions and add functionality if needed)

@brylie brylie modified the milestone: Sprint 23 Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants