Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refs #26346 - Move tasks table to react router #445

Merged
merged 1 commit into from Oct 31, 2019
Merged

Refs #26346 - Move tasks table to react router #445

merged 1 commit into from Oct 31, 2019

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Jul 30, 2019 •
edited

@theforeman-bot
Copy link
Member

@MariaAga, this pull request is currently not mergeable. Please rebase against the master branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream master

This message was auto-generated by Foreman's prprocessor

@MariaAga MariaAga changed the title Move tasks table to react router Refs #26346 - Move tasks table to react router Jul 30, 2019
@theforeman-bot
Copy link
Member

@MariaAga, this pull request is currently not mergeable. Please rebase against the master branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream master

This message was auto-generated by Foreman's prprocessor

adamruzicka
adamruzicka reviewed Jul 30, 2019
View reviewed changes
app/views/foreman_tasks/layouts/react.html.erb Outdated Show resolved Hide resolved
webpack/ForemanTasks/Components/TasksTable/TasksTableSelectors.js Outdated

const formatDate = date =>
date
? new Date(date).toLocaleString('en-GB', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoded locale? Shouldn't it be retrieved from the environment or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didnt want there to be a confusion with the search text as the date for 'en-US' would be MM/DD/YYYY in the table, but it can't be searched in this format

@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch from bb09eb1 to f02df95 Compare July 30, 2019 16:19
@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch 2 times, most recently from 2c0230d to 7e7acf1 Compare July 31, 2019 14:48
@adamruzicka
Copy link
Contributor

Needs a rebase

@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch 8 times, most recently from ea0b5f3 to 0d29dd6 Compare August 19, 2019 12:25
@MariaAga MariaAga mentioned this pull request Aug 22, 2019
Merged
@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch from 0d29dd6 to b7b9e24 Compare August 26, 2019 11:48
@waldenraines
Copy link

@adamruzicka I think this is ready for a review now.

@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch from b7b9e24 to 0711a5d Compare September 1, 2019 11:26
@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch from 0711a5d to ac87a40 Compare September 9, 2019 14:35
@MariaAga MariaAga mentioned this pull request Sep 15, 2019
Merged
@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch 2 times, most recently from 31978f4 to e968c68 Compare September 25, 2019 13:57
sharvit
sharvit suggested changes Sep 26, 2019
View reviewed changes
Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

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

Thank you @MariaAga

webpack/ForemanTasks/Components/TasksTable/TasksTablePage.js Outdated Show resolved Hide resolved
webpack/ForemanTasks/Routes/ForemanTasksRoutes.js Outdated Show resolved Hide resolved
@sharvit
Copy link
Contributor

sharvit commented Sep 26, 2019

Needs a rebase

@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch from 6244663 to e39073f Compare October 3, 2019 08:49
@sharvit
Copy link
Contributor

sharvit commented Oct 3, 2019

Please notice the testing error

@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch from e39073f to f52451d Compare October 3, 2019 09:40
sharvit
sharvit approved these changes Oct 6, 2019
View reviewed changes
Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

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

Great work @MariaAga LGTM and works as expected 👍

@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch from f52451d to 78a63cc Compare October 22, 2019 14:28
sharvit
sharvit approved these changes Oct 23, 2019
View reviewed changes
Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

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

Thanks @MariaAga code wise it makes sense.
Waiting for @adamruzicka to run and test it because I could not atm.

@MariaAga
Copy link
Member Author

@adamruzicka looks like this PR(theforeman/foreman#6936) will be in soon and will change the React Router behavior in Foreman plugins, so I think we should wait for it

@adamruzicka
Copy link
Contributor

Agreed, I'll keep an eye out for it

@MariaAga MariaAga force-pushed the feat/tasks-table-search-react branch from 78a63cc to 859d2a4 Compare October 31, 2019 13:38
sharvit
sharvit approved these changes Oct 31, 2019
View reviewed changes
Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

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

Thank you @MariaAga, looks good and working well.

adamruzicka
adamruzicka approved these changes Oct 31, 2019
View reviewed changes
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Works nicely

@adamruzicka adamruzicka merged commit 0cfa8ea into theforeman:master Oct 31, 2019
@adamruzicka
Copy link
Contributor

Thank you @MariaAga for the PR and @waldenraines, @LaViro and @sharvit for reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ron-Lavi Ron-Lavi Ron-Lavi left review comments

@sharvit sharvit sharvit approved these changes

@adamruzicka adamruzicka adamruzicka approved these changes

Assignees
No one assigned
Labels
Needs testing
Projects
None yet
Milestone
No milestone
6 participants
@MariaAga @theforeman-bot @adamruzicka @waldenraines @sharvit @Ron-Lavi