SVA-258/NativeBaseProjectList #131
Conversation
There was a problem hiding this comment.
Nice work @CatMac228! Have spotted some things, but they're all basically small things so hopefully not loads to do
| > | ||
| <ProjectSummary project={item} /> | ||
| </ProjectListItem> | ||
| <Card margin="2"> |
There was a problem hiding this comment.
From looking at Figma, the edge of the card should line up horizontally with the edge of the segmented picker above, whereas the card has extra spacing around it at the moment here. So I think the margin setting here should be changed to marginY="2" (i.e. vertical margin only, not horizontal) and then I think it'll line up (though do check).
There was a problem hiding this comment.
Did not know about X & Y margins! The spacing was driving me mad 😄
| <Text fontSize="xs">{item.client}</Text> | ||
| <ColouredTag title={item.role} /> | ||
| {Boolean(item.buddying) && ( | ||
| <> |
There was a problem hiding this comment.
Small thing but you don't need the <> and </> here, as the HStack works as a top-level element
| import { useDispatch, useSelector } from 'react-redux' | ||
| import styled from 'styled-components/native' | ||
| import { EventSearch } from './EventSearchContainer' | ||
| import { EventSearch } from '../../Containers/EventSearchContainer' |
There was a problem hiding this comment.
Small thing but the syntax we normally use to reference a different top-level directory inside app/src is '@/Containers/EventSearchContainer'
| onSearchButtonPress={() => navigate(screens.search[params.type], '')} | ||
| /> | ||
| <VStack | ||
| _dark={{ backgroundColor: 'bgDarkMode.100' }} |
There was a problem hiding this comment.
I think you can remove these _dark and _light settings here as the default does the same?
| space={4} | ||
| padding={4} | ||
| > | ||
| <Heading fontSize="md">Projects List</Heading> |
There was a problem hiding this comment.
From looking at the NativeBase docs for Heading, I think the recommended way to change a heading is using the size attribute rather than fontSize (works differently to e.g. <Text>)
There was a problem hiding this comment.
Looking great @CatMac228 -- just one thing I spotted with the bottom of the last card being cut off when I see it in on my Android emulator. Have put in a suggestion you could try, if that doesn't work or you can't figure it out and need some help, do give me a shout if you want.
| @@ -116,18 +130,62 @@ const List: FC<ListProps> = ({ data, mode, options, searchScreen, type }) => { | |||
| return ( | |||
| <FlatList | |||
There was a problem hiding this comment.
The one last thing I've spotted is that the bottom of the last item in the list is being cut off.
I think (but am not certain) that you might need to set a maxHeight for the FlatList using an approach similar to what's being done in ProjectRegisterInterest -- define a height for the stuff that's above the list (like line 37), calculate the height of the window (line 158) , then set a maxHeight of one minus the other (like line 171 except that uses minHeight whereas I think you want to use maxHeight here).
There was a problem hiding this comment.
thank you for spotting, that's an embarrassing miss on my part! 😄 messed about with it there and it is kind of working but there's a bit of white space above the nav icons now...not sure if that's okay or not
(red line = empty space)
There was a problem hiding this comment.
No worries -- obviously good to spot as much as you can yourself, but this partly why we do reviews 🙂
That looks better, thanks. (Could increase the height of the list ever so slightly if you think it needs it.) If you want to push the change I can re-review.
There was a problem hiding this comment.
@CatMac228 that's looking cut off on my emulator?
There was a problem hiding this comment.
oh no it looks so different on IOS 😢
There was a problem hiding this comment.
they seem to be behaving the same way when I use minHeight but not maxHeight 🤔
There was a problem hiding this comment.
It's possible I'm wrong about maxHeight and it should be minHeight -- was just a suggestion off the top of my head. If you've got a version that works do push again and I can try it my end too. I'm travelling this afternoon but should be able to get online again this evening.
There was a problem hiding this comment.
No probs! Will remove the maxHeight bit for now although the list will be cut off slightly at the bottom. Wonder if it could be related to these issues facebook/react-native#23693 &facebook/react-native#36447 ?
Description
Fixes # SVA-258
Type of change
Please delete options that are not relevant
Testing locally
Checklist
IOS:
Android:
