-
Notifications
You must be signed in to change notification settings - Fork 8
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
View lenses #16
View lenses #16
Conversation
react: require("react"), | ||
'@chakra-ui/react': require('@chakra-ui/react'), | ||
'@emotion/react': require('@emotion/react'), | ||
'@emotion/styled': require('@emotion/styled'), | ||
'framer-motion': require('framer-motion') |
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.
Just making a note here: basically eventually we would want people to choose which packages are bundled into their lenses as opposed to forcing them to bundle these. Need to figure out how that could be done.
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.
Agreed! Probably with the exception of React itself, for now.
This'll mean we ultimately want to remove the Chakra provision for the doc component, too.
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.
Actually, now that I think of it, this doesn't actually force these packages on lens creators. This just exposes packages we're already using in the Tiles app so that other people can use them.
More of an issue might be that people want to use systems which aren't compatible with Chakra, and anything else we're passing to the component. This echoes what I said above about removing ChakraProvider so that all lens components operate in a neutral environment.
Even still, I suspect lens creators may still want to go as raw as possible and go with whatever TIles has in the app, because this reduces the bundle size of the remote lens.
import React from 'react' | ||
import { FaChevronLeft, FaChevronRight } from 'react-icons/fa' | ||
import { usePagination } from 'use-pagination-firestore' | ||
import LoadingTableRows from './components/LoadingTableRows' | ||
import db from './firebase' | ||
import moment from 'moment' | ||
|
||
interface DocList { |
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.
Removes unused DocList interface
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.
Assorted notes:
Odd commit loading behavior
I noticed some odd behavior when the following happens:
- You go to a Doc with commits, and set the commit to something other than the initial,
- You go to another Doc (via the search bar) -> the "loading commit" indicator keeps spinning until you select the first commit. I suspect that this is because we don't clear the "loadedCommit" variable if we're browsing to a new doc via the search functionality. Should probably fix within this PR.
Lenses sometimes flash
E.g. I'm looking at document kjzl6cwe1jw148ptxvefz496legyw0kkd01yhvvgqyt310aa5bzrds5kc2w049i
. Selecting "Blog Post Lens 1" causes a UI flash but Lens 2
doesn't. Let's note to investigate (doesn't have to be for this PR).
...More to come as I go through.
Do you think the commit issue is caused by this work? If not, maybe we can leave it for a separate fix. Also would prefer to leave the lens flashing issue to another PR. |
Not sure tbh |
This PR enables users to view lenses on documents that have been added to the LensMarketIndex.
Also worth reviewing the associated Ceramic Streams.
I deleted package-lock.json without thinking – apologies if it causes any issues your side @isidorosp – won't happen again 🙏🏽.