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

Update screenshots on docs intro page #419

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

Conversation

ChristopherGS
Copy link

Updated screenshots to show new UI and removed Beta button

@ChristopherGS ChristopherGS changed the title Update screenshots docs intro page Update screenshots on docs intro page Sep 11, 2024
docs/index.md Outdated
@@ -225,9 +225,13 @@ and it'll end up as structured data in our platform ready to be queried.
For example, using data from the `User` model above, we could list users from the USA:

```sql
SELECT attributes->'result'->>'name' as name, age(attributes->'result'->>'dob') as age
SELECT
Copy link
Author

Choose a reason for hiding this comment

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

when I ran this query, I got this error:
image

So I updated with a query that does work

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally significantly prefer the original example. I suggest:

SELECT attributes->'result'->>'name' as name, extract(year from (attributes->'result'->>'dob')::date) as "birth year"
FROM records
WHERE attributes->'result'->>'country_code' = 'USA'

to demonstrate some fancy-looking SQL. Calculating the age would be better, but apparently datafusion doesn't support extracting years from durations.

Copy link
Author

Choose a reason for hiding this comment

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

updated

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (12a29ac) to head (8ef5dbb).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #419   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          131       131           
  Lines         9696      9696           
  Branches      1266      1266           
=========================================
  Hits          9696      9696           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

cloudflare-workers-and-pages bot commented Sep 12, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8ef5dbb
Status: ✅  Deploy successful!
Preview URL: https://853e1490.logfire-docs.pages.dev
Branch Preview URL: https://cs-update-docs-for-ga-1.logfire-docs.pages.dev

View logs

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Screenshots looks fine.

.gitignore Outdated
Comment on lines 12 to 14

# intellij
.idea
Copy link
Member

Choose a reason for hiding this comment

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

I prefer for ppl to use their own global .gitignore on IDE specific files.

Suggested change
# intellij
.idea

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the harm? Does .gitignore need to be clean?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't accept this on the projects I maintain cause it opens space for ppl to keep adding stuff. I prefer correctness. The gitignore for a project should only contain what is generated from the project itself but shouldn't be committed.

Copy link
Author

Choose a reason for hiding this comment

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

removed

@alexmojaki
Copy link
Contributor

It's not important, but I think future screenshots should probably have a profile pic in them.

Copy link
Contributor

Choose a reason for hiding this comment

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

The mixture of with and without service names doesn't look very good on the left, nor does unknown_service on the right (although other people probably wouldn't notice that bit). It'd also be nice if the service names weren't truncated to example-s... and example-a.... How about just api and worker?

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@alexmojaki
Copy link
Contributor

Also it'd be nice if "Find the needle in a stacktrace" had a screenshot with an actual stacktrace/traceback.

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