Conversation
n3op2
requested review from
jonmattgray and
mattdean-digicatapult
as code owners
|
I've tried a login API call on demo 2 and it has this error: |
|
Demo 2 isn't functioning correctly, think there's something wrong with state? If I login successfully with |
|
@jonmattgray the close bug has been fixed as well |
mattdean-digicatapult
left a comment
mattdean-digicatapult
left a comment
There was a problem hiding this comment.
I'm going to be honest I'm in two minds on this one. The feature add here is really useful but unfortunately you've tied it to a move to storing everything in a global Context which I'm really struggling with. Lack of separation of concerns is not good and this is a definite quality regression. I'm therefore not sure I'm going to approve this even if you address the specific comments I'm making, but obviously that's a good start. I may end up looking at rebasing this into several separate changes to bring in the feature in a more controlled way.
| {...{ | ||
| ...state, | ||
| ProgressBar: ({ update, readDemo }) => ( | ||
| <ProgressBar update={update} readDemo={readDemo} /> | ||
| ), | ||
| }} |
There was a problem hiding this comment.
I honestly don't know what this does. Can we simplify this syntactically to reduce the amount of spreading? It looks likes it could be:
<Modal
type={'readDemo'}
update={update}
ProgressBar={({ update, readDemo }) => (
<ProgressBar update={update} readDemo={readDemo} />
)}
{...state}
/>
| <Help | ||
| theme={theme} | ||
| content={helpContent} | ||
| showContentState={showHelp} | ||
| setShowContentState={setShowHelp} | ||
| /> |
There was a problem hiding this comment.
what has happened to Help? Looking at these changes it looks like it's not there unless we're running in aarch64 i.e. isMorello !== true
There was a problem hiding this comment.
it's below since it's using absolute positioniong anyway so moved it one level
|
|
||
| const renderActions = ({ update, readDemo }) => { | ||
| // giving default value to avoid bugs in case this is undefined | ||
| const renderActions = ({ update, type = 'readDemo', ...props }) => { |
There was a problem hiding this comment.
type is a really ugly solution to this. Separating the Context into two would be much more scalable and useful. I get this is not simple
There was a problem hiding this comment.
totally agree, but I don;'t want to grow this story any more, happy to add a task for breaking down context.
3 participants
What
Mainly adding a new button (console - please refer to screenshots) along with the new asset (img for icon). However, went a little beyond and made
Modal.jscomponents decoupled fromread-demo. There are other changes making UI components a bit more friendly for cross demos.Q: No sure about the icon, maybe it should be within login box?
Changes
read-demoe2e test update to be inline with shared modalScreenshots