Skip to content
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

conditional orderbook tab and button #2888

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SirSaltyy
Copy link
Collaborator

One thing to note is that logged out mobile users still can't see the orderbook. This was an intentional choice as there wasn't a good spot to put it and I don't think a logged out mobile user cares that much.

Signed out desktop users, non-eligible sweeps signed-in desktop users, and non-eligible sweeps signed-in mobile users can now see the orderbook (although this is limited to binary markets rn - which should be fine since those are the only sweeps markets).

Copy link

vercel bot commented Sep 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2024 11:11pm
prod ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2024 11:11pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview Sep 19, 2024 11:11pm

@IanPhilips
Copy link
Collaborator

Could you post a screeny?

Copy link
Collaborator

@mantikoros mantikoros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable, but uncertain about design / layout

Copy link
Collaborator

@IanPhilips IanPhilips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost done! still some odd bits in there

size="lg"
onClick={() => {
setShowOrderBook((prev) => !prev)
if (!showOrderBook) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this clause for?

}
)

const filteredTabs = tabs.filter(Boolean)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this filter

const unfilledBets =
useUnfilledBets(liveContract.id, { enabled: isOrderBookTabActive }) ?? []

if (orderBookTabIndex >= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this? Why isn't the order book simply in the tabs array as the content object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I'm confused I thought this as well as the clause u asked about above in a comment made up the logic that makes it so useUnfilledBets only gets the unfilledbets once the conditions are met and prevent them from loading with the page

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if totalBets>0, a tab.title === 'Order book' is in the tabs, and therefore the orderBookTabIndex.content is set to the <OrderBookPanel/>, so really the only useful clause is if the totalBets is >0, so why not just stick the orderbookpanel in the tabs? and the unfilled bets will either be empty or full if the hook is enabled (aka the orderBookTabActive)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants