AppRepoList v2 #1915
AppRepoList v2 #1915
Conversation
andresmgot
force-pushed
the
appRepoListV2
branch
from
5cf6309 to
bdbea5f
Compare
| const closeModal = () => setModalOpen(false); | ||
| const { errors } = useSelector((state: IStoreState) => state.repos); | ||
| // Primary by default | ||
| const isPrimary = primary !== false; |
There was a problem hiding this comment.
This looks like it's handling the possibility of an undefined for primary as it's optional. In which case, why not just define the default above in the function args as primary = true, ... (see https://stackoverflow.com/a/32596200 )
| <Modal showModal={modalIsOpen} onModalClose={closeModal}> | ||
| {errors.create && ( | ||
| <Alert theme="danger"> | ||
| Found an error creating the repository: {errors.create.message} |
There was a problem hiding this comment.
"An error occurred while creating the repository: {...}"?
| <> | ||
| {errors.fetch && ( | ||
| <Alert theme="danger"> | ||
| Found an error fetching repositories: {errors.fetch.message} |
There was a problem hiding this comment.
I'm guessing you're just re-using the existing messages, so fine to leave as is, but normally an application doesn't find an error, an error occurs... hence suggesting "An error occurred while fetching repositories: {...}" etc.
There was a problem hiding this comment.
Good point. Yes, I have been using this form of Error found in all places. I'll open a new PR to fix all.
| const dispatch = useDispatch(); | ||
|
|
||
| const handleResyncAllClick = async () => { | ||
| // Fake timeout to show progress |
There was a problem hiding this comment.
Why are we faking showing progress when the actual repo is being refreshed? IMO either we shouldn't pretend and instead just use a user info "Repository refresh requested. It may take x minutes to complete.", or it should show actual progress? Again, fine without, just wondering what the longer-term plan is for this "progress".
There was a problem hiding this comment.
It's just a hot fix because right now, there is no feedback for this button so if you click, it seems that nothing happens. Ideally, this should report information back and show the real progress but we don't have that info. Another option would be to disable the button while sending the request to resync the repo but that is so fast that people won't be able to notice. That's why I think adding this feedback is better from a UX. In any case, will add a comment with the desirable behavior.
| @@ -31,7 +36,7 @@ export const defaultStore = mockStore(initialState); | |||
| // getStore returns a store initialised with a merge of | |||
| // the initial state with any passed extra state. | |||
| export const getStore = (extraState: object) => { | |||
| const state = { ...initialState }; | |||
| const state = cloneDeep(initialState); | |||
There was a problem hiding this comment.
Ouch - did you find that by tests writing state then seen in other tests?
There was a problem hiding this comment.
Yep, {...object} only performs a shallow copy, keeping references to inner objects:
> a = {a: 1, b: {c: 2}}
{ a: 1, b: { c: 2 } }
> b = {...a}
{ a: 1, b: { c: 2 } }
> b.b.c = 3
3
> a
{ a: 1, b: { c: 3 } }
There was a problem hiding this comment.
Yep yep, I realise, but also realised that I wrote that code the other week and was saying "ouch" because you may have had to find that bug the hard way (ie. tests failing because of contaminated test data)... hopefully you just noticed it while passing by and didn't suffer as a result :)
There was a problem hiding this comment.
sorry, I found this with weird test results :P but it was easy to spot
* AppRepoList v2 * Review
Description of the change
First PR for adding support for the App Repo list management view:
Since this view is quite complex, this PR only contains the code related to the repository table and the different buttons. I will send other PRs for:
Note that the buttons in the table are "outlined" rather than "flat" because I found a weird behavior: it's not possible to set the color to red ("danger") for flat buttons, the parameter gets ignored.