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

PLANNER-1416: Patternfly React UI #66

Closed

Conversation

danielefiungo
Copy link
Contributor

No description provided.

@yurloc yurloc changed the title Patternfly react ui PLANNER-1416: Patternfly React UI Feb 6, 2019
Copy link
Member

@yurloc yurloc left a comment

Choose a reason for hiding this comment

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

First round of review, a few style issues and typos.

Can we reuse background PNGs and the SVG filter from react-core where they are already bundled (see node_modules/@patternfly/react-core/dist/styles/? I think BackgroundImage should work somehow out of the box without needing to supply your own images.

We need to come up with better names (OVR prefix adds no meaning to component names).

optaweb-vehicle-routing-frontend/src/containers/OVR.tsx Outdated Show resolved Hide resolved
optaweb-vehicle-routing-frontend/src/routes/Depots.tsx Outdated Show resolved Hide resolved
optaweb-vehicle-routing-frontend/src/routes/Depots.tsx Outdated Show resolved Hide resolved
optaweb-vehicle-routing-frontend/tslint.json Show resolved Hide resolved
optaweb-vehicle-routing-frontend/src/routes/Depots.tsx Outdated Show resolved Hide resolved
optaweb-vehicle-routing-frontend/src/index.tsx Outdated Show resolved Hide resolved
@danielefiungo
Copy link
Contributor Author

We need to come up with better names (OVR prefix adds no meaning to component names).

Any ideas?

@@ -60,7 +60,7 @@ const TspMap: React.SFC<ITspMapProps> = ({
center={center}
zoom={zoom}
onClick={clickHandler}
style={{ width: '100vw', height: '100vh' }}
style={{ width: '100%', height: '100vmin' }}
Copy link
Member

Choose a reason for hiding this comment

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

Let's use 100% height as well. It gives the expected result at least on my machine. The map fits nicely in to the available space in both directions, no scroll bars.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, there are some issues with height: 100%. style={{ width: '100%', height: 'calc(100vh - 216px)' }} works best for me, although there is the calc() function again 🙁. I think we need to find a way how to create a container that takes all available vertical space (window height minus navigation bar).

<OVRTheme>
<OVRThemeConsumer>
{({ components }) => (
<Router>
Copy link
Member

@yurloc yurloc Feb 13, 2019

Choose a reason for hiding this comment

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

@danielefiungo Are you aware that we have a BrowserRouter inside another BrowserRouter? Looks unintentional to me.

Bacuse in App.tsx we already wrap OVR by BrowserRouter:

<BrowserRouter>
  <OVR />
</BrowserRouter>

Copy link
Member

Choose a reason for hiding this comment

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

The OVR component is redundant. Let's:

  1. Move routing from OVR into App.
  2. Connect Route to store to avoid passing its props from the App.

Feel free to give it a try, otherwise I'll do that after this PR is merged. I'll also change the component structure from components vs. containers to package by feature (see proposal in #59).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually is related to connected component, still investigating how to do it in correct way with TS ( https://reacttraining.com/react-router/web/guides/redux-integration ).

withRouter need to receive correct types temp fixed in next commit

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if that helps, but what if we disconnect App.tsx? It doesn't have to be connected because we can connect ConnectionError itself and all of the route components can (and I think they should) connect themselves. So it would be simply

export default function App() { // unconnected
  return (
    <BrowserRouter>
      <OVRTheme>
        <React.Fragment>
          <ConnectionError /> // connected
          <Background /> // this has theme consumer inside
          <Page header={<OVRHeader />}> // connected (tenant select will need to access store)
            <PageSection variant={PageSectionVariants.default}>
              ...
              <Route // connected
                path="/route"
                exact={true}
                render={() => <RoutePage />}
              />
            </PageSection>
          </Page>
        </React.Fragment>
        )}
      </OVRTheme>
    </BrowserRouter>
  );
}

Copy link
Member

Choose a reason for hiding this comment

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

Just curious if this could avoid the problems with Router + Redux integration. I have a working proof of concept of this ^^^ locally and I don't observe any issues.

@yurloc
Copy link
Member

yurloc commented Feb 13, 2019

We need to come up with better names (OVR prefix adds no meaning to component names).

Any ideas?

I'll figure out the names later, we can merge without that when the most important issues are resolved.

But I think that simple names like Header, Navigation, Toolbar should do.

@yurloc yurloc closed this Feb 23, 2019
@yurloc
Copy link
Member

yurloc commented Mar 4, 2019

Superseded by #71.

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