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

Cookbook chapter 2 #47

Merged
merged 14 commits into from
Jul 8, 2021
Merged

Cookbook chapter 2 #47

merged 14 commits into from
Jul 8, 2021

Conversation

jonathom
Copy link
Member

@jonathom jonathom commented Jul 1, 2021

Chapter 2 is ready for review 🎉

Things that still need to be done:

  • navigation to reach it via the website menu @m-mohr
  • complete code check @jonathom
  • squash PR @jonathom
  • Fix sidebar headings (go deeper, also into h3/h4) @m-mohr

Any opinions and reviews are very welcome!

@jonathom jonathom requested a review from m-mohr July 1, 2021 11:39
@jonathom jonathom self-assigned this Jul 1, 2021
@m-mohr
Copy link
Member

m-mohr commented Jul 2, 2021

Thanks, I'll look at this next week. I'm wondering though whether we should simply merge this with "chapter 1"?

@jonathom
Copy link
Member Author

jonathom commented Jul 6, 2021

I'm wondering though whether we should simply merge this with "chapter 1"?

That's a hard question. While discoverability would improve, I think that having chapters is a good way to compartmentalize all the different functions and use cases that are flying around. Additionally, the examples sometimes reuse previous code, which I think becomes less clear with growing chapter size.

@m-mohr
Copy link
Member

m-mohr commented Jul 6, 2021

The issue is that the space in the menu is limited and I should not add multiple chapters into the menu.

It could still be two chapters in a single document, maybe? The TOC is generated anyway on the left side...

@jonathom
Copy link
Member Author

jonathom commented Jul 6, 2021

so this isn't a didactic issue, it's a menu design issue :D do you want to revise now or after I merge the chapters? I will have to make some changes.

@m-mohr
Copy link
Member

m-mohr commented Jul 6, 2021

Yes, more or less. For now, I'd propose to make two chapters, but have them on the same page for now. Would be great if you could do it in this PR.

</template>
</CodeSwitcher>

![NDVI image of the AOI](../cookbook/pellworm_ndvi.png)
Copy link
Member

Choose a reason for hiding this comment

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

Should we use the figures and captions as in the data cube intro? I think they look nicer and have linked captions :-)

<figure>
    <img src="./datacubes/dc_flat.png" alt="Datacube flat representation: The 12 imagery tiles are now laid out flat as a 4 by 3 grid (bands by timesteps). All dimension labels are depicted (The timestamps, the band names and the x, y coordinates).">
    <figcaption>This is the 'raw' data collection that is our example datacube. The grayscale images are colored for understandability, and dimension labels are displayed.</figcaption>
</figure>

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes good idea! I also thought about adding some more images to chapter one. Am I assuming correctly that I should then delete some from chap 2 (e.g. the NDVI one, not really needed) as to not overload the one document that's gonna come out of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot from 2021-07-07 10-25-54
Per default this renders the examples a bit bigger than needed. Is there a way to access textwidth? When I hardcode width to some percent of the page it doesn't scale well with zooming into the page.

Copy link
Member

Choose a reason for hiding this comment

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

We need to find the sweet spot here. As long as images are compressed good enough, I leave it up to you to decide :)

Copy link
Member

@m-mohr m-mohr Jul 7, 2021

Choose a reason for hiding this comment

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

I'll think about it, should be doable in CSS without any change from your side. So just use the figures and I'll try to solve that they don't upscale.

Copy link
Member Author

Choose a reason for hiding this comment

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

done on my end.

@m-mohr
Copy link
Member

m-mohr commented Jul 6, 2021

I just realized that the images are rather large (also for the urk image and some getting started images).

I think we should try to stick to a maximum of roughly 500kb per image. Maybe it's better to compress them all as JPEG files?

@jonathom
Copy link
Member Author

jonathom commented Jul 6, 2021

Copy that. Will take care of it tomorrow.

@m-mohr
Copy link
Member

m-mohr commented Jul 6, 2021

Tested it with a getting started image: Reduced from 1,8 MB PNG to 0,3 MB JPEG with those settings:

image

Compressed the existing images, but not the ones in this PR.

Note to me: Squash this PR later to get rid of the large files in the git history.

@m-mohr
Copy link
Member

m-mohr commented Jul 6, 2021

Thanks, very well written (although I added a lot of comments about minor things).

@jonathom
Copy link
Member Author

jonathom commented Jul 7, 2021

Now another issue with TOC arises: only one # heading is printed but there are three (openEO cookbook, chapter 1, chapter 2) and no #### level headings are included (so moving everything one # down excludes subsections).. anything we can do about this?

@m-mohr
Copy link
Member

m-mohr commented Jul 7, 2021

I think there's a config option, let me check...

@m-mohr
Copy link
Member

m-mohr commented Jul 7, 2021

Try to add this at the start of the markdown file:

---
sidebarDepth: 4
---

@jonathom
Copy link
Member Author

jonathom commented Jul 7, 2021

doesn't seem to work. maybe because of local server instead of via web?

@m-mohr
Copy link
Member

m-mohr commented Jul 7, 2021

Seems we need to set another config option globally, I'll take care of it after merging.

@jonathom jonathom merged commit 2ad7271 into master Jul 8, 2021
@jonathom jonathom deleted the cb_chap2 branch July 8, 2021 15:49
@m-mohr
Copy link
Member

m-mohr commented Jul 8, 2021

Good work!

  • I've checked the images and they are not exceeding 100% of their size so it's fine if they show up larger as they don't upscale and get pixelated.
  • I could not get the headers working yet, either I did something wrong or our version doesn't support more levels yet. I'll open a separate issue to keep track as I'll likely not be able to solve it soon. See Cookbook: Show h4 headers in TOC #49

@jonathom
Copy link
Member Author

jonathom commented Jul 8, 2021

Thanks!

  • I was more concerned about the images for aesthetic reasons, as now they're a bit big, standing out more than I thought they would. But that's ok, took some getting used to.
  • Thanks for investigating

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.

2 participants