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

More PyTorch theme fixes #236

Merged
merged 22 commits into from
Jul 1, 2020
Merged

More PyTorch theme fixes #236

merged 22 commits into from
Jul 1, 2020

Conversation

lethosor
Copy link
Contributor

Closes #230

@lethosor lethosor added bug Bug fixes documentation Documentation work enhancement Code enhancement labels Jun 30, 2020
@lethosor lethosor self-assigned this Jun 30, 2020
Copy link
Contributor

@tylerganter tylerganter left a comment

Choose a reason for hiding this comment

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

So the top nav is still not quite right aligned:
Screen Shot 2020-06-30 at 2 53 53 PM

And this is more of opinion, but I liked having the home button better than having a FiftyOne link. Even though I knew what i was looking for it took me a minute to find the FiftyOne link which isn't as attention grabbing as it ideally could be

Otherwise looks good!

@lethosor
Copy link
Contributor Author

Is that Firefox? Your screenshot is wider than my physical screen, so I suspect there's a wider screen size where the content widens that I didn't handle.

@lethosor
Copy link
Contributor Author

lethosor commented Jun 30, 2020

Yeah, I think that must be Firefox-specific - it looks fine to me in Chrome with a 3000px-wide window (this is just the left side of it):

image

Thanks for remembering Firefox, though. I'll see what I can do.

@tylerganter
Copy link
Contributor

Looks like the same problem to me in your screen shot. Try FiftyOne should be roughly right above Shortcuts

@lethosor
Copy link
Contributor Author

No, my intention was to align it to the right side of the content, not the sidebar (which is basically where it was before the theme change), since not all pages have a right sidebar. It's about 50-100px further to the left in your screenshot than mine, though.

Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

Nice work @lethosor!!

Can you squeeze in a couple small changes before you merge?

  • Can you increase the font size on the smallest elements like the sidebars, the Docs > XXXX links at the top, etc? They're pretty small right now. On https://voxel51.com/docs/fiftyone currently, the main body is 16pt and those other things are either 16pt or 15pt. On this branch, they're all 14pt. I'd try all 16pt, and, if that looks weird, then try 15pt.
  • If it's easy, can you make the line number sidebars on the code blocks fixed width? On my local build they seem to be variable width and often get too wide. EG user_guide/using_dataset for the most apparent example:

Screen Shot 2020-07-01 at 4 06 02 PM

@brimoor
Copy link
Contributor

brimoor commented Jul 1, 2020

No, my intention was to align it to the right side of the content, not the sidebar (which is basically where it was before the theme change), since not all pages have a right sidebar. It's about 50-100px further to the left in your screenshot than mine, though.

I like Alan's goal of aligning with the RHS of the content, not the sidebar. I observe some minor strangeness when adjusting the size of my browser window. There are some transition points when the header jumps 50-100px to the right as it seems Tyler observed

@brimoor
Copy link
Contributor

brimoor commented Jul 1, 2020

So the top nav is still not quite right aligned:
Screen Shot 2020-06-30 at 2 53 53 PM

And this is more of opinion, but I liked having the home button better than having a FiftyOne link. Even though I knew what i was looking for it took me a minute to find the FiftyOne link which isn't as attention grabbing as it ideally could be

Otherwise looks good!

FYI- I changed the ToC entry for the homepage from FiftyOne to Overview. I think that's more descriptive; also, my two cents are that the V51 logo already functions as a "home" button (albeit for the main website, not the docs), so I'd rather not introduce a second home button for the docs; only one home button per page.

@lethosor
Copy link
Contributor Author

lethosor commented Jul 1, 2020

Addressed comments from #236 (review) - the header positioning only seems to be an issue somewhere between 900px and 1200px wide, and isn't worth delaying other fixes for now

@lethosor lethosor merged commit 4beafff into develop Jul 1, 2020
@lethosor lethosor deleted the more-theme-fixes branch July 1, 2020 21:53
kaixi-wang pushed a commit that referenced this pull request May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fixes documentation Documentation work enhancement Code enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancements/bugfixes for PyTorch theme
3 participants