project enhancements #22
Conversation
| /** | ||
| * -------------------------------- | ||
| * Import Material UI Components | ||
| * -------------------------------- | ||
| */ | ||
| // Style Components |
There was a problem hiding this comment.
whats the motivation behind this file?
Why not import directly from mui?
There was a problem hiding this comment.
@Amoodaa Nothing special, just thought of it in case any of these components might need some customization later, that was my only concern.
There was a problem hiding this comment.
Ah! Mui allows for insane customizability using the theme, so i wouldnt reexport it for that reason
also its better to remove the customizability so you can force the team to not override the styles, unless agreed on case by case basis
| @@ -0,0 +1,11 @@ | |||
| export const BASE_URL = 'http://localhost:8080'; | |||
There was a problem hiding this comment.
base url isnt just the localhost, how are we dealing with that on front end, also i thought you were trying to unify front and back routes definition into 1 file
There was a problem hiding this comment.
i thought you were trying to unify front and back routes definition into 1 file.
- yes, that was my intention, as I thought it might be easier to track the modifications of endpoints on both sides.
| import { ActionReducerMapBuilder } from '@reduxjs/toolkit'; | ||
| import { searchYoutube } from './thunk'; | ||
| import { SearchState } from './types'; | ||
|
|
||
| export const extraReducers = (builder: ActionReducerMapBuilder<SearchState>) => { | ||
| builder | ||
| .addCase(searchYoutube.pending, state => { | ||
| state.state = 'loading'; | ||
| }) | ||
| .addCase(searchYoutube.fulfilled, (state, action) => { | ||
| state.state = 'idle'; | ||
| state.youtubeSearchResult = action.payload.data; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Imho this is an overkill extraction, the slice file is supposed to be logic heavy, i agree with taking out the thunks from same file, but the slice will have its own simpler reducers, just add these extra ones there as well i think
but you know whats amazing here? nice work on the typing! i hate when people extract and dont type!! this is amazing
There was a problem hiding this comment.
Roger that! will reunion the reducers family then.
| import { YoutubePlaylistSearch, YoutubeVideoSearch } from 'youtube.ts'; | ||
|
|
||
| export interface SearchState { | ||
| youtubeSearchResult: YoutubeVideoSearch | YoutubePlaylistSearch | null; | ||
| state: 'idle' | 'loading' | 'failed'; | ||
| } | ||
|
|
||
| export type YoutubeSearchForm = { searchTerm: string; type: 'playlists' | 'videos' }; |
There was a problem hiding this comment.
me wouldnt have extracted that, but it's ok, keep it!
for me, sometimes it's not worth the mental overhead of extracting stuff, you know switching files and looking here and there to make 1 small change
I try to make all my changes be nice and friendly for developers (caring for DX)
There was a problem hiding this comment.
You got a very legitimate point!
But here I might have a question; when it comes to refactor your code, how would you distinguish between an overkill cleanup/premature optimization and a separation of concerns?
| }, | ||
| ); | ||
| }, | ||
| extraReducers: builder => extraReducers(builder), |
There was a problem hiding this comment.
bring this back in, because the extra reducers for this slice wont increase nor the core reducers
the slice is good enough to have all the reducers in them as you'd have a big "reducer.ts" when using plain redux
In this PR:
utils/validation.util.tsassets/fonts&assets/themeexamplePlaylistSearchdata ontoutils/constants/playlistSearch.constant.tscreateAsyncThunkandextraReducersfrom the slice files, and moved each one onto a standalone file.MaterialUIcomponent and moved the imports there.endpoints.constants.tsfile instead of writing it directly wihtincreateThunkAsyncfunctions