-
Notifications
You must be signed in to change notification settings - Fork 793
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
Improve encoding section #2735
Improve encoding section #2735
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks good to me, I suggested a few clarifications. Regarding whether we need a separate encoding page for, I am not sure. Adding the full extent of what is in the VegaLite encoding pages does seem useful but also like a lot of work. I think it would be nice to have the general intro text for each encoding and maybe one simple example for each of them (could even be the same one from the example gallery that is currently linked in the overview table)? Could that be a good middle ground (it would also move and break up the lengthy options table)
If we don't create separate pages for each encoding, I think the headings "Encoding Channels" and "Encoding channel options" should be moved to last on the page. These are more like reference and they don't feel like narrative documentation that I am expected to read. I think the presence of long tables is a general problem with the Altair docs that I remember confused me when starting out. It is not clear that there will be more narrative content under these tables so we should move them last on each page or wrap them in a details
tag so that they can be expanded on demand and it is easy to see the narrative content. Another notable case of this is the "Top-Level Chart Configuration" page.
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
What specifically do you mean with “encoding”? I’m a bit confused with the naming around the encoding concepts. An encoding channel such as “color” or an encoding channel option such as “axis”? Initially, this also confused me in the vega-lite docs although I don’t have a better suggestion as I think it makes sense content-wise. Under transforms, we have sub pages about actual transforms, under marks the same, but for encoding the vega-lite documentation has sub pages for encoding channel options and not the channels themselves. Creating separate pages per options would probably involve rewriting some of the existing pages I linked in the first comment. Good point about the tables. I’ll look into this next and will also consider the “Top-Level Chart Configuration” page. |
Collapsing longer tables looks nice but I find it a bit less convenient as I can no longer search for a keyword such as a parameter name on the page without first expanding the tables. What do you think about
|
Great point, I think the VegaLite doc structure in this regard has been secretly confusing for me as well, but I have gotten used to it. I think explicit mention of the encoding channels with an example of each would be great and clearly separate them from the channel options. What do you think of this structure?
I was surprised that these are not searchable so I looked around a bit. It seems like expanding detail tags on search was made part of the official web spec last year whatwg/html#6466 and that it works in chrome already while there is an open issue for it in firefox. With that background, the fact that there is still a search button for the docs in general, and that the "Top level chart config" page would benefit a lot from it (and I can't think of another workaround there), I think it could still be beneficial to have tables collapsed inside details tag by default, what do you think?
Agree with this move close #2572 |
…ning. Other minor improvements
Regarding the restructuring of the encoding page. I like your suggestion of clearly separating channels and channel options. I pushed a first suggestion, you can preview them at https://binste.github.io/altair-docs/user_guide/encodings/index.html I for now sticked to the existing content and did not rewrite the tables for the channels into sections for each channel including the example, same for the options. For the channels I see that this could be nice but I'd prefer if this can be tackled in a separate PR. Happy to create an issue to keep track of this enhancement. For the options, I'm not sure how to approach this. How it's currently set up is great if you want to see which options are supported by a channel. This information is lost if we have one section for each option. Of course we could then include in that section which channels support it but I'd find that somewhat less convenient to navigate. Does that make sense for you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! I have a few minor comments inline. Regarding the separate pages for all channels and channel options, I will say that I do really like the current re-structuring and although it is possible that there are some more benefits from creating separate pages, it seems like the benefit is less clear so we might get into the territory of diminishing returns. I opened #2763 to track it, but I'm happy merging this when the minor comments I just left are addressed.
@@ -566,3 +566,145 @@ it is rendererd, you can set ``width`` or ``height`` to the string ``"container" | |||
Note that this will only scale with the container if its parent element has a size determined | |||
outside the chart itself; For example, the container may be a ``<div>`` element that has style | |||
``width: 100%; height: 300px``. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This page is so nice to read with the collapsed tables! I noticed there were just a few sections that were missing a sentence introducing the tables, could we add these:
Can we use the same wording for projection and selection as for scale ranges ("Projections can be configured using..."):
The table in the Header section is missing a leading sentence, could we add something like "Additional properties are summarized in the following table:"
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Co-authored-by: Joel Ostblom <joelostblom@users.noreply.github.com>
Great, thanks for the suggestions and for setting up the separate issue! I incorporated all changes and agree with you that separate subpages could be nice but maybe not the most value-adding contribution right now. Ready to merge from my side :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for improving this section of the docs, I think it will be a big help to many people!
datum
andvalue
. Addresses Document the differences betweenvalue
anddatum
? #2572detail
channelkey
anddescription
channels as they are not very relevant for Altair and not properly documented anywayI also considered creating separate encoding pages as in the vega-lite documentation. However, it felt like the most important content is already covered in the existing encoding section (with the additions mentioned above) as well as in Customizing Visualization, Scale and Guide Resolution, and Top-Level Chart Configuration