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

Dependencies update. Restructure each page to be modularized. Refactor Scene Page and more #61

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from

Conversation

Jlu18
Copy link
Member

@Jlu18 Jlu18 commented Jan 25, 2022

Changes

Bump node engine version from 10.13 to 12.18

Update to following dependencies:

  • react
  • react-admin
  • mui (aka. material-ui)
  • react-scripts

Added two new dependencies:

  • react-ace (for displaying code)
  • ra-compact-ui (for a custom tab layout)

Removed unused dependencies

  • react-bootstrap
  • ra-data-json-server
  • ra-input-rich-text

File Structure Change:
Resource Pages

  • For each resources (ie. scene, course, google login, etc), there's a new directory contains all their components inside it
  • Each components are separated into individual file (ie. Show, Edit, Create, etc.)
  • Those components are then group together in index.js and exported as a module

Scene Page

  • New tabbed in SceneList specficially for example scenes
  • Edit, Show now contains Scene Settings and code are displayed as same style in frontend MYR

components folder

  • Moved to page directory and has two new file that has component that displays data in different ways.

Further TODOs

  • Refactor Course Page
  • Complete documentations
  • Fix warning on install
  • Switch from yarn to npm for consistency
  • Add Linter
  • and couple more stuff that I can't think of right now.

Jlu18 added 20 commits November 15, 2021 15:44
…re the LoginPage class and functions. And change import syntax of mui icons at App.js
…ed Scene Page, by splitting to SceneList and SceneShow. Restructure SceneList so the Scenes are tabbed into user scenes and example scenes. Filter, actions, etc are still WIP.
@Jlu18 Jlu18 marked this pull request as ready for review January 26, 2022 16:44
@Jlu18 Jlu18 requested a review from kdvalin January 26, 2022 16:52
@kdvalin kdvalin requested review from banthedev and jdones01 March 1, 2022 18:47
Copy link
Member

@banthedev banthedev left a comment

Choose a reason for hiding this comment

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

Everything looks great to me I don't want to be nick-picky but there are a few lines commented (as shown below). If they're meant for future reference and not lines you forget to remove then its all good. 👍


// //Extract uid(different from ids) from data we received
// let uids = ids.map(id=>data[id].uid);
// //Filter out the repeated uids and the uids isn't 1;
Copy link
Member

@banthedev banthedev Mar 2, 2022

Choose a reason for hiding this comment

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

Not sure if you've commented these for future reference but if not it would be good to remove these


const CourseFilter = (props) => (
<Filter {...props}>
{/* <TextInput label="Search" source="q" alwaysOn /> */}
Copy link
Member

@banthedev banthedev Mar 2, 2022

Choose a reason for hiding this comment

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

Comments here as well

<Create {...props}>
<SimpleForm toolbar={<RefExCreateToolbar />}>
<TextInput source="functionName" />
{/* <TextInput source="info" /> */}
Copy link
Member

Choose a reason for hiding this comment

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

More commented lines

Copy link

@jdones01 jdones01 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, however, some things are scarce with comments, and some things have been commented out (without a note as to why they are still there and not deleted).

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.

3 participants