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 #13424 - c3 patternfly react implementation #3603

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
6 participants
@gailsteiger
Member

gailsteiger commented Jun 20, 2016

The change can be seen on the statistics page.
This PR depends on the Webpack PR which has been merged.

@gailsteiger

This comment has been minimized.

Show comment
Hide comment
@gailsteiger

gailsteiger Jun 20, 2016

Member

TODO:

  • use template strings
  • resolve conflicts with global window event handlers
  • check links in small donut charts
  • add loading indicator or empty chart while json is loading
  • add error handling (timeout, failure) to API calls
Member

gailsteiger commented Jun 20, 2016

TODO:

  • use template strings
  • resolve conflicts with global window event handlers
  • check links in small donut charts
  • add loading indicator or empty chart while json is loading
  • add error handling (timeout, failure) to API calls
Show outdated Hide outdated app/helpers/application_helper.rb Outdated
Show outdated Hide outdated config/initializers/assets.rb Outdated
Show outdated Hide outdated package.json Outdated
@dLobatog

Some comments on what the Emitter, MountingService & friends do would be great for the casual contributor to understand what's going on. Currently I believe most contributors (including myself) would have a hard time creating components in this fashion, but I suppose comments can help that. Thanks for the effort updating the PR and now with tests (:+1) @gailsteiger

Show outdated Hide outdated package.json Outdated
Show outdated Hide outdated test/lib/has_many_common_test.rb Outdated
numFields: require('./jquery.ui.custom_spinners')
};
window.tfm = Object.assign(

This comment has been minimized.

@dLobatog

dLobatog Sep 20, 2016

Member

Why not just adding reactMounter to the list?

@dLobatog

dLobatog Sep 20, 2016

Member

Why not just adding reactMounter to the list?

This comment has been minimized.

@gailsteiger

gailsteiger Sep 20, 2016

Member
window.tfm = window.tfm || {}; 

is preferred over

window.tfm = { ...}

Otherwise important objects could be easily overridden.

Originally I wrote it as follows:

window.tfm = window.tfm || {};
window.tfm.tools = require('./foreman_tools');
window.tfn.numFields: require('./jquery.ui.custom_spinners'),
window.tfm.reactMounter = require('./react_app/common/MountingService');

Tomer prefers the object syntax so this is a sort of compromise

@gailsteiger

gailsteiger Sep 20, 2016

Member
window.tfm = window.tfm || {}; 

is preferred over

window.tfm = { ...}

Otherwise important objects could be easily overridden.

Originally I wrote it as follows:

window.tfm = window.tfm || {};
window.tfm.tools = require('./foreman_tools');
window.tfn.numFields: require('./jquery.ui.custom_spinners'),
window.tfm.reactMounter = require('./react_app/common/MountingService');

Tomer prefers the object syntax so this is a sort of compromise

Show outdated Hide outdated webpack/assets/javascripts/pages/statistics_page.js Outdated
@@ -0,0 +1,17 @@
import ServerActions from './actions/ServerActions';
export default {

This comment has been minimized.

@dLobatog

dLobatog Sep 20, 2016

Member

Isn't 'API' too generic of a name for an API just for statistics?

@dLobatog

dLobatog Sep 20, 2016

Member

Isn't 'API' too generic of a name for an API just for statistics?

This comment has been minimized.

@gailsteiger

gailsteiger Sep 20, 2016

Member

We will add other API's here until it makes sense to create separate files.

@gailsteiger

gailsteiger Sep 20, 2016

Member

We will add other API's here until it makes sense to create separate files.

} else {
const componentName = components[component].type.name;
// eslint-disable-next-line no-console

This comment has been minimized.

@dLobatog

dLobatog Sep 20, 2016

Member

? If you want to enable console for certain messages ('error'?) feel free to edit the eslintrc to do { "allow": ["error"] }

@dLobatog

dLobatog Sep 20, 2016

Member

? If you want to enable console for certain messages ('error'?) feel free to edit the eslintrc to do { "allow": ["error"] }

This comment has been minimized.

@gailsteiger

gailsteiger Sep 20, 2016

Member

I don't want to enable console globally. The developer should disable it specifically if it needs to be used. That will discourage using console messages.

@gailsteiger

gailsteiger Sep 20, 2016

Member

I don't want to enable console globally. The developer should disable it specifically if it needs to be used. That will discourage using console messages.

Show outdated Hide outdated webpack/assets/javascripts/react_app/actions/StatiscticChartActions.js Outdated
Show outdated Hide outdated ...ack/assets/javascripts/react_app/components/charts/StatisticsChartBox.js Outdated
module ReactjsHelper
def mount_react_component(name, selector, data)
javascript_tag defer: 'defer' do
"$(tfm.reactMounter.mount('#{name}', '#{selector}', #{data}));".html_safe

This comment has been minimized.

@gailsteiger

gailsteiger Sep 20, 2016

Member

not using html_safe breaks the app

@gailsteiger

gailsteiger Sep 20, 2016

Member

not using html_safe breaks the app

This comment has been minimized.

@dLobatog

dLobatog Sep 26, 2016

Member

Have you tried safe_join as houndci says?

@dLobatog

dLobatog Sep 26, 2016

Member

Have you tried safe_join as houndci says?

This comment has been minimized.

@gailsteiger

gailsteiger Oct 5, 2016

Member

@dLobatog yes, I tried the hound suggestion. It breaks the application. @tbrisker said to ignore the error since html_safe is widely used throughout the application.

@gailsteiger

gailsteiger Oct 5, 2016

Member

@dLobatog yes, I tried the hound suggestion. It breaks the application. @tbrisker said to ignore the error since html_safe is widely used throughout the application.

@@ -0,0 +1,5 @@
const c3 = {};

This comment has been minimized.

@ohadlevy

ohadlevy Sep 20, 2016

Member

why is this in / path?

@ohadlevy

ohadlevy Sep 20, 2016

Member

why is this in / path?

This comment has been minimized.

@gailsteiger

gailsteiger Sep 27, 2016

Member

it's the jest default for node_module mocks

@gailsteiger

gailsteiger Sep 27, 2016

Member

it's the jest default for node_module mocks

Show outdated Hide outdated app/models/concerns/has_many_common.rb Outdated
Show outdated Hide outdated test/functional/statistics_controller_test.rb Outdated
Show outdated Hide outdated webpack/assets/javascripts/react_app/actions/StatiscticChartActions.js Outdated
Show outdated Hide outdated webpack/assets/javascripts/react_app/components/common/MessageBox.js Outdated
Show outdated Hide outdated webpack/assets/javascripts/react_app/components/common/MessageBox.js Outdated
@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Sep 20, 2016

Member
Member

ohadlevy commented Sep 20, 2016

@dLobatog

This comment has been minimized.

Show comment
Hide comment
@dLobatog

dLobatog Sep 26, 2016

Member

@ohadlevy It seems like you have to approve the changes.
@gailsteiger Can you rebase? Sorry.. it went out of date again.

From my side, 👍 for merging it, except for the inline CSS styles which I really doubt we need as @tbrisker pointed out.

In any case, for future PRs: a 1000+ LOC change is quite difficult and takes a lot of time to review, no matter how trivial the lines are. There are studies done on the effectiveness of code reviews and pretty much anything above ~300LOC will contain defects no matter how much code review you do, which I pretty much agree with. [1][2][3]

If you look at commits within the last 3 years and a half over 800 LOC - git log --since 2013 --format="%h - %s" --shortstat | grep insertions -B2 | sed 's/--//g' | awk 'NF' | awk 'ORS=NR%2?" ":"\n"' | grep -E '[0-2]*[8-9][0-9][0-9] insertions'[4] - most of them were "code" or something to review, but translations/changelog/templates etc.

Thanks again for keeping up with the PR @gailsteiger - it takes a lot of grit to merge big PRs like this, hopefully we can keep PRs after this much smaller and quicker to review/merge! 😄

[1] http://www.pitt.edu/~ckemerer/PSP_Data.pdf
[2] https://smartbear.com/learn/code-review/what-is-code-review/
[3] http://www.slideshare.net/nnja/how-to-successfully-grow-a-code-review-culture
[4] ignore my clumsiness with awk, I didn't know how to get those stats in an easier way

Member

dLobatog commented Sep 26, 2016

@ohadlevy It seems like you have to approve the changes.
@gailsteiger Can you rebase? Sorry.. it went out of date again.

From my side, 👍 for merging it, except for the inline CSS styles which I really doubt we need as @tbrisker pointed out.

In any case, for future PRs: a 1000+ LOC change is quite difficult and takes a lot of time to review, no matter how trivial the lines are. There are studies done on the effectiveness of code reviews and pretty much anything above ~300LOC will contain defects no matter how much code review you do, which I pretty much agree with. [1][2][3]

If you look at commits within the last 3 years and a half over 800 LOC - git log --since 2013 --format="%h - %s" --shortstat | grep insertions -B2 | sed 's/--//g' | awk 'NF' | awk 'ORS=NR%2?" ":"\n"' | grep -E '[0-2]*[8-9][0-9][0-9] insertions'[4] - most of them were "code" or something to review, but translations/changelog/templates etc.

Thanks again for keeping up with the PR @gailsteiger - it takes a lot of grit to merge big PRs like this, hopefully we can keep PRs after this much smaller and quicker to review/merge! 😄

[1] http://www.pitt.edu/~ckemerer/PSP_Data.pdf
[2] https://smartbear.com/learn/code-review/what-is-code-review/
[3] http://www.slideshare.net/nnja/how-to-successfully-grow-a-code-review-culture
[4] ignore my clumsiness with awk, I didn't know how to get those stats in an easier way

@gailsteiger

This comment has been minimized.

Show comment
Hide comment
@gailsteiger

gailsteiger Sep 27, 2016

Member

@dLobatog would like to handle inline-styles/css discussion in a separate PR. This one is long enough.

Member

gailsteiger commented Sep 27, 2016

@dLobatog would like to handle inline-styles/css discussion in a separate PR. This one is long enough.

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Sep 27, 2016

Member
Member

ohadlevy commented Sep 27, 2016

@gailsteiger

This comment has been minimized.

Show comment
Hide comment
@gailsteiger

gailsteiger Sep 27, 2016

Member

@ohadlevy taking out of this PR defeats the whole point of doing a separate PR. If I take out the inline styles I'll have to replace them with something else. That would essentially be the separate PR.

Member

gailsteiger commented Sep 27, 2016

@ohadlevy taking out of this PR defeats the whole point of doing a separate PR. If I take out the inline styles I'll have to replace them with something else. That would essentially be the separate PR.

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Sep 27, 2016

Member
Member

ohadlevy commented Sep 27, 2016

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Sep 27, 2016

Member

@gailsteiger note that CI tests are failing

Member

ohadlevy commented Sep 27, 2016

@gailsteiger note that CI tests are failing

// destroy chart on close
const ChartModal = ({ show, config, onHide, setTitle, title, id }) => {
function onEnter() {

This comment has been minimized.

@ohadlevy

ohadlevy Sep 27, 2016

Member

question: can't we just use the chart component with a different ID inside a model component? why do we need the duplication?

@ohadlevy

ohadlevy Sep 27, 2016

Member

question: can't we just use the chart component with a different ID inside a model component? why do we need the duplication?

This comment has been minimized.

@gailsteiger

gailsteiger Oct 6, 2016

Member

@ohadlevy I need to draw the chart when the modal opens. Unique problem for this modal because of c3

@gailsteiger

gailsteiger Oct 6, 2016

Member

@ohadlevy I need to draw the chart when the modal opens. Unique problem for this modal because of c3

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Sep 27, 2016

Member

toggling open/close hopefully for travis to pick up testing

Member

ohadlevy commented Sep 27, 2016

toggling open/close hopefully for travis to pick up testing

@ohadlevy ohadlevy closed this Sep 27, 2016

@ohadlevy ohadlevy reopened this Sep 27, 2016

const components = {
StatisticsChartsList: {
type: StatisticsChartsList,

This comment has been minimized.

@ohadlevy

ohadlevy Sep 29, 2016

Member

do we really need the type option? we can probably use the key name instead?

@ohadlevy

ohadlevy Sep 29, 2016

Member

do we really need the type option? we can probably use the key name instead?

This comment has been minimized.

@gailsteiger

gailsteiger Oct 6, 2016

Member

@ohadlevy I think it's cleaner this way. I don't want to treat the key as a value

@gailsteiger

gailsteiger Oct 6, 2016

Member

@ohadlevy I think it's cleaner this way. I don't want to treat the key as a value

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Oct 6, 2016

Member

@gailsteiger one anonying side effect, is that when opening a modal chart, the top bar expands, see:
recorded

Member

ohadlevy commented Oct 6, 2016

@gailsteiger one anonying side effect, is that when opening a modal chart, the top bar expands, see:
recorded

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Oct 9, 2016

Member

merged as 659d49a thanks @gailsteiger !!!

Member

ohadlevy commented Oct 9, 2016

merged as 659d49a thanks @gailsteiger !!!

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