feat/fix to issue #242 #248
Conversation
There was a problem hiding this comment.
Please, fix github workflow issue
https://github.com/djeck1432/spotnet/actions/runs/11984813605/job/33416020314?pr=248
|
@ekumamatthew any updates? |
|
Updating my PR shortly |
There was a problem hiding this comment.
styles don't match figma, please take a look how other modals are implemented already (e.g. notification one on dashboard)
There was a problem hiding this comment.
@ekumamatthew this is still unresolved after latest changes), also - modal should be centered on the screen
|
@ekumamatthew Any updates? |
There was a problem hiding this comment.
@ekumamatthew seems like some of your changes were overwritten after your latest merges, (Form.jsx), please take a look and fix, thanks
|
I made the position function a hook. so that it can be re-useable anytime |
There was a problem hiding this comment.
So, functionally-wise everything is fine.
But as I already pointed there are issues with modal's styling.
ER (Figma):
AR (Your implementation):
You should do it exactly the same as in Figma. As I already suggested you already have the same modal on dashboard for notifications, just use the same styles as there...
|
@ekumamatthew put your background file to public folder and use it in css like that |
|
Font colour... okay. I'll look at the figma again |
|
I had a look at the figma text color its same here |
|
@ekumamatthew |
|
@ekumamatthew this page is completely broken now, there are everything duplicated, probably it was wrong resolvment of merge conflict |
|
@ekumamatthew don't use |
|
@ekumamatthew Also, now I realise that you don't need background image for popup at all, in figma it is just a demonstration that on the background should be actual page, not some image, so remove |
|
@ekumamatthew also, please resolve merge conflicts with main and make sure everything works, (last time I've checked font color were ok), just need to merge it properly and correctly use your modal component and not |
|
@ekumamatthew Please, fix test cases |
|
ive sync with origin. i'm getting along with the UI and the project better ... also the ui is not so mobile fit. I can fix this if I'm pointed to the UI.. so u can |
|
@ekumamatthew now the whole Form page is completely broken... can you please revert css changes that do not relate to close position popup? |
|
@ekumamatthew please, text me in dm, I'm going to unassign you, sorry, this critical task and we need to finish it right away |
| return ( | ||
| <div className="form-content-wrapper"> |
There was a problem hiding this comment.
removing this container cause broken ui on the page..
|
@ekumamatthew also seems like background on the main page is broken now on this branch for some reason. |
|
@ekumamatthew please fix failed CI build |
|
@ekumamatthew you should build your app before you commit and make sure it works. |
Fix #242