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

Fix: reorganize index structure for users #188

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

lwasser
Copy link
Member

@lwasser lwasser commented Mar 1, 2024

this is a pr from @sneakers-the-rat that was sent to my fork. i'm moving it here so we can work on it once the other pr #181 is merged in a few hours!

@lwasser
Copy link
Member Author

lwasser commented Mar 4, 2024

@sneakers-the-rat it looks like i tried to start rerouting your pr's here. is this one we should rebase and look at together? or can you reroute pr's that were on my fork to the main branch here?

@sneakers-the-rat
Copy link
Contributor

Ya ya will do shortly ♥

@willingc
Copy link
Collaborator

@sneakers-the-rat @lwasser What is the next step for this PR? Rebase?

@@ -69,7 +69,7 @@ h1 {
color: var(--pyos-h1-color);
}
h2 {
margin-top: 80px;
margin-top: 1em;
Copy link
Member Author

Choose a reason for hiding this comment

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

@sneakers-the-rat we can merge this but i have a question. i see

em, px which is fixed and rem used. and i never know when to use what approach. i'm curious what your thoughts are!! :) just a learning moment for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically the way I think about it is px is rarely the "right" thing for responsive because what really matters most of the time is relative size - 1px is very different as a proportion of screen on different displays. But it is definitely the easiest and most predictable to reason about.

em and rem are both relative units, but em is relative to the current (parent) font size and REM is relative to the root size. So REM when you might use the class in multiple contexts but want the thing to look the same, em when you would want it to scale. I just default em.

With padding/margins specifically I think em is easier to think about bc I am looking at the page and can eyeball "I need about a text height space there" vs. I have no idea what that is in px. And the scale isnt necessarily linear but the spacing you need scales with the text size anyway so you need to do fewer fine tuning things over time. When you see a page where all the spacings are subtly wonky its usually because the sizes use px/other absolute unit (and also maybe not using sass in an era before easy css variables and calc)

@@ -56,7 +56,7 @@ Community docs

Publish your docs
```
## _new_ Tutorial Series: How to Create a Python Package
## _new_ Tutorial Series
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
## _new_ Tutorial Series
## _new_ Tutorial Series: Create a Python package

Copy link
Member Author

Choose a reason for hiding this comment

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

How about a middle ground. i just know that header tags matter in search results. And people skim

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way! All these are always just suggestions.

@@ -100,7 +100,7 @@ by the community now! Join our community review process or watch development of
:::::


## Python packaging ecosystem overview & best practices
## Packaging
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
## Packaging
## Python Packaging for Scientists

Copy link
Member Author

@lwasser lwasser left a comment

Choose a reason for hiding this comment

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

@willingc thanks. this can be merged except i want two small changes. i have no idea what's happening - i can't approve the pr. but i'm an admin.
Screenshot 2024-03-20 at 9 35 09 AM

@lwasser
Copy link
Member Author

lwasser commented Mar 20, 2024

ok i can't approve it but i should be able to make edits to the files. i'm really confused

@lwasser
Copy link
Member Author

lwasser commented Mar 20, 2024

Let's merge and i'll make the changes myself in a separate pr

@lwasser lwasser merged commit 7fcd88a into pyOpenSci:main Mar 20, 2024
4 checks passed
@lwasser lwasser mentioned this pull request Mar 20, 2024
@sneakers-the-rat
Copy link
Contributor

Works for me. Wonder what happened !?

@willingc
Copy link
Collaborator

Thanks for doing this @lwasser and @sneakers-the-rat. That's the first that I have seen that as an admin. If it happens again, let me know.

@lwasser
Copy link
Member Author

lwasser commented Mar 20, 2024

so odd. Here is my theory @sneakers-the-rat @willingc ... i think i actually opened this pr because Jonny had opened the pr against my other pr and i had missed them (they were in my fork). So i didn't want to lose jonny's ideas and re-opened the pr against the main branch. But the pr is still from jonny's fork.

and i wonder if i didn't check "allow maintainer edits" or something? which is weird because i'm an owner of this repo but i created a PR from a fork that i do not own. i'm really not sure :) but i just made those 2 tiny edits to the headers in a separate pr so all good. the changes here as fantastic!! Thank you Jonny for submitting this and being so thoughtful about user experience of our guide!

@lwasser
Copy link
Member Author

lwasser commented Mar 20, 2024

and @willingc thank you for helping with handling all of these pr's. i am definitely behind so this is hugely helpful to be able to pop in and review / merge or just for you to merge things that are ready to go!!

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