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.

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

Fixes #26349 - Create tasks details page in react #425

Merged
merged 1 commit into from Aug 15, 2019

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Jun 12, 2019

The reload Button now only re-fetches the task data instead the whole html
The time calculation has slightly changed and now follows http://cldr.unicode.org/ format

wip for:

  • missing tests
  • ruby 'leftovers'

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.

Left some comments inline, rubocop is not too happy about these changes.

@MariaAga MariaAga force-pushed the feat/tasks-details-page branch 3 times, most recently from 779ab22 to 2ba59b7 Compare June 12, 2019 14:43
@MariaAga
Copy link
Member Author

Fixed the rubocop issues

@MariaAga MariaAga force-pushed the feat/tasks-details-page branch 2 times, most recently from 2a80bd8 to 865468d Compare June 13, 2019 08:45
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.

Did a little bit of testing, encountered a couple of issues. For a task which started at Thu, 13 Jun 2019 09:30:29 UTC +00:00 and ended at Thu, 13 Jun 2019 09:30:42 UTC +00:00:

This changes how the details page looks a tiny bit, it is especially noticeable on non-standard display sizes, see https://imgur.com/a/KCLtclk

@MariaAga
Copy link
Member Author

@adamruzicka I couldn't reproduce the date issues, can you attach the API data from the task:
/foreman_tasks/api/tasks/TASK_ID/details
?

@adamruzicka
Copy link
Contributor

@MariaAga
Copy link
Member Author

@adamruzicka I loaded all of your data and still didn't encounter this issue, can you maybe re-check?

@adamruzicka
Copy link
Contributor

It seems it works in chrome (left, google-chrome-stable-75.0.3770.90-1), but doesn't work in firefox (right, firefox-67.0-4)
image

@MariaAga
Copy link
Member Author

Apparently Firefox can't format dd-mm-yyyy date like chrome, and that's the format that Ruby sends. Fixed this and the styling issues, thanks for the help @adamruzicka !
I still didn't receive the console error (not in Firefox or in Chrome)

@adamruzicka
Copy link
Contributor

adamruzicka commented Jun 18, 2019

When the window is narrow, every other row is right aligned.

When the window is wide, the content doesn't cover the entire available space. I'm not saying that's bad, just that it is different from the previous behavior.

https://imgur.com/a/LWQbHRy

@MariaAga
Copy link
Member Author

Thanks for bringing this to my attention! It looks wrong to me so I changed it and I think it looks better now

@MariaAga MariaAga changed the title [WIP] Fixes #26349 - Create tasks details page in react Fixes #26349 - Create tasks details page in react Jun 18, 2019
@MariaAga MariaAga force-pushed the feat/tasks-details-page branch 2 times, most recently from ce92481 to 0aae550 Compare June 18, 2019 15:54
@MariaAga MariaAga marked this pull request as ready for review June 18, 2019 15:58
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.

When you're looking at a stopped task and turn on auto reload, it keeps refreshing. Before this PR it would refresh once, figure out the task is already stopped and won't change in the future so it stopped reloading.

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.

I encountered an odd issue.

  1. (prerequisite) Have 2 tasks, one which is running and one which is stopped
  2. Go to tasks index
  3. Select running task
  4. Click back in the browser
  5. Select the stopped task

Actual result: The task renders for a moment, but that is suddenly replaced by data from the running task.
Expected result: The task renders and stays.

I can reproduce it somewhat reliably so if you need any data feel free to reach out to me.

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.

Two more things. Also needs a rebase

@jeremylenz
Copy link

Apparently Firefox can't format dd-mm-yyyy date like chrome, and that's the format that Ruby sends. Fixed this and the styling issues, thanks for the help @adamruzicka !
I still didn't receive the console error (not in Firefox or in Chrome)

Hi @MariaAga

I'm on Chrome (75.0 on Fedora 30) and I'm also getting the "Invalid date" issue.

I think the problem is in webpack/ForemanTasks/Components/TaskDetails/Components/TaskHelper.js

In the formatDate function when you do

return new Date(date.replace(/-/g, '/'));

It's replacing the timezone offset with a /, which makes it an invalid date. Like this:

date screenshot

@MariaAga
Copy link
Member Author

MariaAga commented Aug 8, 2019

@jeremylenz thanks for noticing! I changed the replace function to only replace twice now (and not all the occurrences)

@MariaAga
Copy link
Member Author

@adamruzicka fixed the odd issue. The problem was that the interval for re fetching the data was set twice but stopped once

@MariaAga MariaAga force-pushed the feat/tasks-details-page branch 4 times, most recently from 2641936 to f452ea9 Compare August 13, 2019 11:18
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.

Rubocop is not happy. Also can you tell if the failures on travis are related to changes introduced by this PR?

@MariaAga
Copy link
Member Author

failures on travis are unrelated to this PR, and fixed rubocops issues

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.

Looks good to me and works as expected, ACK

@adamruzicka adamruzicka merged commit 4efdfaa into theforeman:master Aug 15, 2019
@adamruzicka
Copy link
Contributor

@MariaAga thank you and sorry for the delays.

@sharvit, @jeremylenz thank you for testing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants