-
-
Notifications
You must be signed in to change notification settings - Fork 421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial research module layout #1110
Add initial research module layout #1110
Conversation
Visit the preview URL for this PR (updated for commit 40cfe5c): https://onearmy-next--pr1110-research-module-fron-ai8bkc36.web.app (expires Thu, 08 Apr 2021 23:12:46 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
niceee! |
|
||
const GridBox = styled(Box)` | ||
display: grid; | ||
grid-template-columns: 2fr 1fr 1fr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personally I like css grid and media queries, but in case you weren't aware there's an added option included in rebass components to provide widths for the theme breakpoints (https://rebassjs.org/layout). You'll see in a lot of the howto components code like <Flex px={4} py={4} flexDirection={'column'} width={[1, 1, 1 / 2]}>
where the width array corresponds to sizes at the various theme breakpoints (at least I think that's what it does! I try to stay away from frontend where I can)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice touch! i was kinda fixed on the grid idea cause i used to work with bootstrap a lot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's a kind of weird niche that these rebass components are trying to fill. I see the benefit if you're trying to keep consistency across all screens/components, but like most js and css libraries/frameworks if you know what you are doing as a developer you can usually achieve the same outcomes using more vanilla-style approaches. I don't think we're super opinionated, as long as there is a reasonable level of consistency or standard approaches so that someone else can make changes in the future if required, happy with whatever way devs find themselves working best.
import { IUploadedFileMeta } from 'src/stores/storage' | ||
import ImageGallery from './ImageGallery' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for refactoring this into a shared component, makes a lot of sense to do so. Feel free to make any other changes you see as sensible for tidying up component structures or sharing code between components
import { MOCK_UPDATES } from 'src/mocks/research.mocks' | ||
import { NotFoundPage } from 'src/pages/NotFound/NotFound' | ||
import { ResearchStoreContext } from 'src/stores/Research/research.store' | ||
import { Loader } from '../../../components/Loader/index' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this also just the src/components
folder? if so probably easier to import it that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, using auto-import is tricky sometimes
{ | ||
...update, | ||
// TODO - insert metadata into the new update | ||
_id: Math.random().toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it happens this should be handled automatically in src\stores\databaseV2\index.tsx
- basically all db writes get automatically populated with standard _created, modified, _id metadata so individual stores don't need to worry about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, also if you ever did want to use Math.random to generate a random string you can also include a radix in the string method. E.g. Math.random().toString(36)
is pretty common, as base36 would use 10 digits (0-9) and 26 letters of alphabet (a-z), so you get an output like 0.pwqjb68tqqr
. Take the substring from digit 2 and you get a quick unique id pwqjb68tqqr
. There's still a few limitations with this however, as strings won't always be uniform length if ending in 0s (which would just be omitted) - small discussion on this if you scroll down here: https://stackoverflow.com/questions/1349404/generate-random-string-characters-in-javascript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the reviews! And nice tip on the Math.random().toString()
usage - wasn't used to that. A question here, I did try to insert the updates without the metadata, the meta fields weren't automatically populated by the DB though, I think it's because the updates are an array inside of the research object which is in fact populated with the metadata. Do we have to store the updates in a separate collection and reference them from the research object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that makes a lot more sense, I had been thinking about individual docs and not data stored within the research object array. I think we can go about it either way, either creating a new subcollection doc each time there is an update, or just storing everything in a single array in the research object itself. If we store things in the object itself then we will want to manually add some metadata as you started to do. Happy for you to proceed however you think best and we can always decide if we want to refactor later
What's there looks really good to me. For the metadata you actually don't need to worry as that's all handled by automatically by the common db methods mentioned in the comment above. So you can just remove the code to manually add. Otherwise from a dev side I'm happy to merge and continue to build from here. Assuming you're happy with the design implementation so far @davehakkens? (can follow-up next steps in separate issue) |
Added a couple of changes for the suggestions, the only issue is with the update DBDoc metadata populating - as I responded to your comment the db isn't populating it automatically when concatenating a new raw update to the array :( |
took the liberty to add a |
Nice! Was gonna ask if you could also add it to the pr-labeller action that automatically tags things based on the code that changes but can see you've already done it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the updates, looks like a nice base to build from, merging
PR Type
PR Checklist
master
branch mergedDescription
Adding the first layout for research module listing, details and updates. Also finished setting up the folder structure and mock data for researches.
Still missing:
Git Issues
Closes #1080 and closes #1079 - probably should create some more issues for the above points
Screenshots/Videos