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 scenario overview #88

Merged
merged 10 commits into from
Feb 16, 2024
Merged

Conversation

fudler
Copy link
Collaborator

@fudler fudler commented Jan 19, 2024

Let us create some common ground!

@fudler fudler requested a review from l-1squared January 19, 2024 14:57
Signed-off-by: Christian Oertel <[email protected]>
@l-1squared
Copy link
Contributor

stuff is weirdly red here

Copy link
Contributor

@l-1squared l-1squared left a comment

Choose a reason for hiding this comment

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

Had a first look, but need to make a thorough review

new/package.json Show resolved Hide resolved
new/src/App.tsx Outdated Show resolved Hide resolved
new/src/components/casesTable/CasesTable.tsx Outdated Show resolved Hide resolved

const root = ReactDOM.createRoot(document.getElementById("root") as HTMLElement);

i18n.use(initReactI18next).init({
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you dump the i18n?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See above haha. Let's just add it again.

@l-1squared
Copy link
Contributor

still quite red :(

new/src/components/ScenarioOverview/ScenarioOverview.tsx Outdated Show resolved Hide resolved
</ListItem>
<ListItem sx={{ paddingTop: 0.1, paddingBottom: 0.1 }}>
<ListItemText primary={
<Link href="http://localhost:3000" underline="none" sx={{ color: 'inherit' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

We clearly need to start adding routing...

Copy link
Contributor

Choose a reason for hiding this comment

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

not part of this PR obv.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah.

</div>
</Grid>
<Grid item xs={12} md={11}>
<Grid container direction="column">
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it would be too bad, if we place the subgrid in a different component?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll keep Todos in the code as a starting point for the... well, workshop!

<List>
<ListItem sx={{ paddingTop: 0.1, paddingBottom: 0.1 }}>
<ListItemText primary={
<Link href="http://localhost:3000" underline="none" sx={{ color: 'inherit' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

we really should not use the base url here. I just ended up on the playwright react app.
just using the path might work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, I'd just keep it and provide a comment for future use.

/>
</ListItem>
</List>
<ListItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

again, "forEach" could be put to good use here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

First, let's keep it like this. The fix here might be a good introduction for someone in the workshop.

@fudler fudler force-pushed the refactor_scenario_overview branch 3 times, most recently from 884ee67 to c1e68f5 Compare February 2, 2024 14:34
Signed-off-by: Christian Oertel <[email protected]>
@fudler fudler force-pushed the refactor_scenario_overview branch from c1e68f5 to 8786f5e Compare February 2, 2024 14:41
@fudler fudler requested a review from l-1squared February 2, 2024 14:44
@fudler
Copy link
Collaborator Author

fudler commented Feb 2, 2024

So, the red stuff is gone. I left some of your comments "unresolved" in the sense of "we use them as basis for the workshop".

@fudler fudler merged commit b0c103d into React-Migration Feb 16, 2024
7 checks passed
@fudler fudler deleted the refactor_scenario_overview branch February 16, 2024 08:36
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.

2 participants