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

New watermark on SVG graphs. #2776

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tarehart
Copy link
Contributor

Updating the watermark:

  • In the background, behind the graph data
  • Larger, so you can still read it if the graph data is in front of it
  • Included the logo

Related suggestions in discord:

image

image

There's another file, probability-needle.tsx, that has the old watermark; I haven't touched it because I think it looks fine on the probability needle.

Copy link

vercel bot commented Aug 11, 2024

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

Name Status Preview Comments Updated (UTC)
dev ❌ Failed (Inspect) Aug 13, 2024 9:33pm
prod ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 13, 2024 9:33pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview Aug 13, 2024 9:33pm

Copy link

vercel bot commented Aug 11, 2024

@tarehart is attempting to deploy a commit to the Manifold Markets Team on Vercel.

A member of the Team first needs to authorize it.

@tarehart
Copy link
Contributor Author

Hard for me to guess why dev failed deployment, could someone please provide logs?

@jahooma
Copy link
Collaborator

jahooma commented Aug 12, 2024

It was a flaky error for our /stats page, which is unrelated!

Copy link
Collaborator

@ingawei ingawei left a comment

Choose a reason for hiding this comment

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

Hello! I see the vision behind this change, but am hesitant to approve because there are just too many instances where this kind of watermark will be entirely illegible, kind of defeating the purpose:
image

@tarehart
Copy link
Contributor Author

tarehart commented Aug 13, 2024

Good call, that example scribbled it out worse than I imagined. Philosophically I think the data should take priority, but I guess we could explore putting the watermark left of the time range controls so they never conflict, does that sound good?

@tarehart
Copy link
Contributor Author

It'll take me a while to experiment with a watermark near the time range controls since there are a bunch of code paths to change and test, and I'm a little less confident about what would look good. Might be a week before I make time.

Should we consider merging this in the meantime as an incremental improvement?

@tarehart
Copy link
Contributor Author

Side note, as an outside contributor with no GOOGLE_APPLICATION_CREDENTIALS_DEV, it's hard to test the contract page. I've resorted to temporarily using a regular db here rather than an adminDb:

const adminDb = await initSupabaseAdmin()
const contract = await getContractFromSlug(adminDb, contractSlug)

What's the reason for this distinction, and is adminDb actually needed for the contract page, which appears to be read only?

@tarehart
Copy link
Contributor Author

tarehart commented Sep 4, 2024

Looking at the mobile dimensions again, I'm skeptical that I can make the alternative placement look good in the available space.

I think this change as it stands is a clear improvement, and I suggest merging it. The people suffer https://discord.com/channels/915138780216823849/1269095533956563014/1280347805101326429

@IanPhilips
Copy link
Collaborator

It looks good on the binary graphs but is basically invisible on the MC graph
Screenshot 2024-09-25 at 10 30 40 AM

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.

4 participants