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

Refactor debug dashboard #226

Merged
merged 17 commits into from
Apr 8, 2019
Merged

Refactor debug dashboard #226

merged 17 commits into from
Apr 8, 2019

Conversation

roderickhsiao
Copy link
Collaborator

@roderickhsiao roderickhsiao commented Mar 28, 2019

  • Refactor debug dashboard to use react component
  • Use React.lazy to load component for code splitting

@roderickhsiao roderickhsiao changed the title [WIP] Refactor debug dashboard Refactor debug dashboard Mar 28, 2019
@roderickhsiao
Copy link
Collaborator Author

roderickhsiao commented Mar 28, 2019

I looked into #23 and found it could be hard to render the debug message without adding a wrapper the the i13nNode having position: relative, which most likely will break the component style if we add a new html structure.

This PR still render the debug container in pure Javascript at the bottom of the page as an overlay to the I13nNode, but moving the component into React so we can better manage the code.

Also use Suspense to load the component so the code could be split out.

I could add some unit test but since we probably want to move to jest, I probably will wait until then to write some component unit test

@roderickhsiao
Copy link
Collaborator Author

roderickhsiao commented Mar 28, 2019

@kaesonho @redonkulus

@roderickhsiao
Copy link
Collaborator Author

Screen Shot 2019-03-28 at 4 50 45 PM

All the style and functionality should remain the same as the original code.

Breaking change will be we will require higher version of React in order to do React.lazy

@roderickhsiao
Copy link
Collaborator Author

the build fails on saucelab connection, I just notice that the grunt task never successfully run before (it has syntax error) so just directly skip and return pass. I fixed the syntax error and now it seems to have difficulties connect to sauce lab.

@redonkulus are you able to check if the saucelab account react-i13n still working?

@roderickhsiao
Copy link
Collaborator Author

I changed to used to saucelab key I applied for subscribe-ui-event and the tests passed :)

@roderickhsiao
Copy link
Collaborator Author

roderickhsiao commented Apr 2, 2019

Screen Shot 2019-04-01 at 9 32 53 PM

I take it back, the test doesn't seems to finished (same for subscribe-ui-event) probably the way we wrote the functional tests are deprecated or something

Copy link
Contributor

@kaesonho kaesonho left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

As we discussed offline, I think we still have to mount debug node on root to preventing breaking styles.

Thank you for also fixing the test page and saucelab tests 🥇

@@ -170,7 +186,7 @@ module.exports = function (grunt) {
main: './<%= project.functional %>/bootstrap.js'
},
output: {
path: './<%= project.functional %>/'
path: path.resolve(process.cwd(), projectConfig.functional)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thx for fixing this, I noticed this is broken for a while

@kaesonho
Copy link
Contributor

kaesonho commented Apr 5, 2019

If we want to make this simpler, we can maybe do Portals here. so that we can still keep dashboard to be component-like and mount it into document root.

something like: in createI13nNode

render() {
  // ...
  const debugDashboard = DEBUG_MODE ? <DebugDashboardContainer i13nNodeRef={this.i13nNodeRef}/> : null;
  return (
    <>
      {debugDashboard}
      {i13nComponent}
    </>
  )
}

and inside DebugDashboardContainer, create a new node in root and use ReactDOM.createPortal to mount the DebugDashboard to that node.

BTW calling it dashboard looks weird here ha. not sure why I named it this way that time 😅

@roderickhsiao
Copy link
Collaborator Author

I'll check if that is doable

@kaesonho
Copy link
Contributor

kaesonho commented Apr 7, 2019

🔥 But I'd say this PR is good enough to be merged that if we want we can follow up with another PR.

@roderickhsiao
Copy link
Collaborator Author

I'll cut a branch (alpha-1) and prepare for release log later today.

@roderickhsiao
Copy link
Collaborator Author

3.0.0-alpha-1

Features

  1. Dashboard components now renders by React
  2. Sub component will be loaded via React.lazy which will be code-Splitted

Build Enhancement

  1. Fix functional tests page
  2. Fix sauce lab tests

Breaking changes

  1. Requires React 16.6.0 (React.lazy)

@roderickhsiao roderickhsiao merged commit f102532 into yahoo:master Apr 8, 2019
@roderickhsiao roderickhsiao deleted the dashboard branch April 8, 2019 20:14
@roderickhsiao roderickhsiao mentioned this pull request Apr 8, 2019
17 tasks
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.

None yet

3 participants