Lee Zhao Yi Charles - Project 1 #39
Lee Zhao Yi Charles - Project 1 #39charlesleezhaoyi wants to merge 11 commits intorocketacademy:mainfrom
Conversation
…odule Form. Pending fixes to add to Module List
… ability to set localStorage. Pending fixes for State initialisation for userAction and grade, and implementation for fetching from localStorage
… for local storage. Refactoring for user experience using bootstrap in progress
…ault is present to prevent re-rendering and light styling
| } | ||
|
|
||
| handleSubmit = (event) => { | ||
| const { userAction, moduleList, moduleName, grade } = this.state; |
| handleSubmit = (event) => { | ||
| const { userAction, moduleList, moduleName, grade } = this.state; | ||
|
|
||
| console.log("handleSubmit"); |
There was a problem hiding this comment.
never keep console logs in your pushed code. Especially this one serves no purpose
|
|
||
| console.log("handleSubmit"); | ||
|
|
||
| if (moduleName === "") { |
There was a problem hiding this comment.
| if (moduleName === "") { | |
| if (!moduleName) { |
| this.setState({ error: null }); | ||
| } | ||
|
|
||
| console.log("Add Module"); |
| return; | ||
| } | ||
| // Add logic for adding modules | ||
| if (userAction === "Add") { |
There was a problem hiding this comment.
I think since you have a couple of user actions, you could just create functions for those.
addModule(), editModule() etc.
Also, since you are checking against a static string, I would recommend creating ENUM strings for those, so no typos will ever sneak into your code. Example:
const USER_ACTIONS = {
ADD: "Add",
UPDATE: "Update"
}
You could even go further and describe the values as the functions:
const USER_ACTIONS = {
["Add"]: addModule(),
["Update"]: updateModule()
}
Then you don't need to run if/else spaghetti but can just do USER_ACTIONS[userAction] and it will call the relevant function
| </select> | ||
| <Button | ||
| type="submit" | ||
| className={enabled > 0 ? "button-enabled" : "button-disabled"} |
There was a problem hiding this comment.
| className={enabled > 0 ? "button-enabled" : "button-disabled"} | |
| className={enabled ? "button-enabled" : "button-disabled"} |
Above 0 is truthy. Also, enabled should not be a number, but a boolean.
| {/* Display the module list */} | ||
| </form> | ||
| </Card> | ||
| {error ? <div className="error-message">{error}</div> : <div></div>} |
There was a problem hiding this comment.
I would not render an empty div
| {error ? <div className="error-message">{error}</div> : <div></div>} | |
| {error && <div className="error-message">{error}</div>} |
| {error ? <div className="error-message">{error}</div> : <div></div>} | ||
| <h2 className="module-list-title"> Module List</h2> | ||
| <Card> | ||
| <table> |
There was a problem hiding this comment.
I would advise to avoid tables as much as possible. If you need a table-like layout, use divs with flexbox instead.
| <h3>Modules</h3> | ||
| <tr className="module-table-row-item"> | ||
| <ListGroup> | ||
| {moduleList && |
There was a problem hiding this comment.
since moduleList defaults to empty array as per above definition [], this statement will always be true
There was a problem hiding this comment.
Also, if you make the mapped list group item conditional, you should include the ListGroup and the tr tag above into the condition to avoid rendering those without any content
| <div className="navbar navbar-expand-lg navbar-light bg-light"> | ||
| <h3>GPA Calculator</h3> | ||
| </div> |
There was a problem hiding this comment.
Hmmmm I am a bit on the fence, if this really needs an own component, as there is only an h3 being displayed 😅
| padding: 1%; | ||
| height: 2%; | ||
| margin-left: 5%; |
There was a problem hiding this comment.
I would avoid percentages for these properties. What is a 5% margin for example or a 1% padding? This doesn't really make sense and might have odd behaviour
| align-items: left; | ||
| } | ||
| /* Thead */ | ||
| /* TO FIX ALL OF THE FOLLOWING*/ |
There was a problem hiding this comment.
Such comments are kinda redunant :) Mostly we will not do it, and therefore can just not keep this comment but rather keep a task in your todo list
| constructor(props) { | ||
| super(props); | ||
| this.state = { | ||
| enabled: "1", |
| moduleName: "", | ||
| moduleList: JSON.parse(localStorage.getItem("moduleList")) || [], | ||
| grade: "A", | ||
| error: "", |
There was a problem hiding this comment.
nice job on adding validation to your form
2 participants
Project 1 Pull Request