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

Docs: Refactor site navigation bar #12289

Merged
merged 1 commit into from
Feb 18, 2025
Merged

Conversation

manuzhang
Copy link
Collaborator

@manuzhang manuzhang commented Feb 16, 2025

@@ -20,39 +20,69 @@ title: "Terms"

# Terms

### Snapshot
## Catalog
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this, the catalog.md was always a bit lonely as it was the only concepts page.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually favor a little more on keeping catalog.md as a separate page, as the content is more than just terms. just keep this page in the new Specification section. Catalog is an important concept and deserves more details that the content page captured than simple term definition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree catalogs deserve more details but having both catalogs and terms looks confusing to me. Isn't catalog a term?

Copy link
Contributor

Choose a reason for hiding this comment

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

agree that catalog is a term, which is good to be clarified in the terms page. but the content also talked a lot more about implementations. anyway, I can go either way on this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: what about taking Catalog one level up?

Currently its the first subtab under "Terms". Would it make sense to make it its on sub in "Specification"?

Screenshot 2025-02-18 at 9 06 17 AM

site/nav.yml Outdated
- AES GCM Stream spec: gcm-stream-spec.md
- REST Catalog Spec: https://editor-next.swagger.io/?url=https://raw.githubusercontent.com/apache/iceberg/main/open-api/rest-catalog-open-api.yaml
- Implementation status: status.md
- Terms: terms.md
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also be open to bringing this back to its original place. Now it is at the very bottom of the specs, but I think it is nice for folks to understand the concepts without plowing through the specs themselves. Curious to learn what others think.

Copy link
Collaborator Author

@manuzhang manuzhang Feb 17, 2025

Choose a reason for hiding this comment

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

Do you mean keeping it at Project tab while merging Catalog into it?
Actually, these terms are more or less explained in the table spec. Maybe it's valuable as additional references as part of Specification, more like a FAQ?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe Fokko meant moving the Terms page at the top of the Specification section. that would make sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

Top sounds good to me

@RussellSpitzer
Copy link
Member

I think this looks good, but I would request some screenshots of the changes on the PR to demonstrate the differences. I don't think they have to be kept up to date with the review, but it's good to see what we are committing as rendered.

@manuzhang
Copy link
Collaborator Author

Screenshots of the website after the change

Homepage

CleanShot 2025-02-18 at 12 27 41@2x

Project Tab

CleanShot 2025-02-18 at 12 28 23@2x

Specification Tab

CleanShot 2025-02-18 at 12 29 47@2x

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for improving the docs! 💯

I verified the changes by running make serve locally

Copy link
Contributor

Choose a reason for hiding this comment

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

👍
except for concepts/catalog.md, for which the content is merged into site/docs/terms.md, nothing else changed. The tabs are just moved around to under "Specification"

Copy link
Contributor

Choose a reason for hiding this comment

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

ok i also double checked that moving from one tab to another doesn't affect the URL.

For example, "Table Spec" is moved from under "Project" to under "Specification", but the URL is still /spec 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

the only URL that changed (removed) is concepts/catalog

@@ -20,39 +20,69 @@ title: "Terms"

# Terms

### Snapshot
## Catalog
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: what about taking Catalog one level up?

Currently its the first subtab under "Terms". Would it make sense to make it its on sub in "Specification"?

Screenshot 2025-02-18 at 9 06 17 AM

@stevenzwu
Copy link
Contributor

stevenzwu commented Feb 18, 2025

seems like we have enough approvals. I am going to merge this.

any other changes can be followed up separately.

thanks @manuzhang for the contribution and @RussellSpitzer @Fokko @kevinjqliu for the reviews

@stevenzwu stevenzwu merged commit 5e1ce86 into apache:main Feb 18, 2025
2 checks passed
ankurbansal-tradedoubler added a commit to ankurbansal-tradedoubler/iceberg that referenced this pull request Feb 19, 2025
* Site: Learn More to point to Spark QuickStart Doc (apache#12272)

* Build: Bump datamodel-code-generator from 0.27.2 to 0.28.1 (apache#12290)

* Spark 3.5: Fix job description of RewriteTablePathSparkAction (apache#12282)

* Build: Bump io.netty:netty-buffer from 4.1.117.Final to 4.1.118.Final (apache#12287)

Bumps [io.netty:netty-buffer](https://github.com/netty/netty) from 4.1.117.Final to 4.1.118.Final.
- [Commits](netty/netty@netty-4.1.117.Final...netty-4.1.118.Final)

---
updated-dependencies:
- dependency-name: io.netty:netty-buffer
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Build: Bump software.amazon.awssdk:bom from 2.30.16 to 2.30.21 (apache#12286)

Bumps software.amazon.awssdk:bom from 2.30.16 to 2.30.21.

---
updated-dependencies:
- dependency-name: software.amazon.awssdk:bom
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* OpenAPI: Add overwrite option when registering a table (apache#12239)

* OpenAPI: Add optional overwrite when registering table

* simplify to overwrite

* Add the article to the description

Co-authored-by: Eduard Tudenhoefner <[email protected]>

* Update generated python as well

Signed-off-by: Hongyue Zhang <[email protected]>

* Fix import order

---------

Signed-off-by: Hongyue Zhang <[email protected]>
Co-authored-by: Eduard Tudenhoefner <[email protected]>

* Build: Bump mkdocs-material from 9.6.3 to 9.6.4 (apache#12284)

Bumps [mkdocs-material](https://github.com/squidfunk/mkdocs-material) from 9.6.3 to 9.6.4.
- [Release notes](https://github.com/squidfunk/mkdocs-material/releases)
- [Changelog](https://github.com/squidfunk/mkdocs-material/blob/master/CHANGELOG)
- [Commits](squidfunk/mkdocs-material@9.6.3...9.6.4)

---
updated-dependencies:
- dependency-name: mkdocs-material
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Core: Fix Enabling row-lineage during Create Table (apache#12307)

* API: Reject unknown type for required fields and validate defaults (apache#12302)

* API: Fix TestInclusiveMetricsEvaluator notStartsWith tests. (apache#12303)

* Core: Add variant type support to utils and visitors (apache#11831)

* Core: Fix CI: Update tests with UnknownType from required to optional (apache#12316)

* Docs: Refactor site navigation bar (apache#12289)

* Parquet: Implement Variant readers (apache#12139)

* Docs: Add rewrite_table_path Spark Procedure (apache#12115)

* Parquet: Fix errorprone warning (apache#12324)

* Docs: Add Apache Amoro docs (apache#11966)

* Parquet: Fix performance regression in reader init (apache#12305)

* Core: Fallback to GET requests for namespace/table/view exists checks (apache#12314)

Co-authored-by: Daniel Weeks <[email protected]>

* Docs: Fix refs in Apache Amoro docs (apache#12332)

* Revert "Core: Serialize `null` when there is no current snapshot (apache#11560)" (apache#12312)

This reverts commit bf8d25f.

* Parquet: Fix performance regression in reader init (apache#12305) (apache#12329)

Co-authored-by: Bryan Keller <[email protected]>

* Checkstyle: Apply the same generic type naming rules to interfaces and classes (apache#12333)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Hongyue Zhang <[email protected]>
Co-authored-by: Danica Fine <[email protected]>
Co-authored-by: Manu Zhang <[email protected]>
Co-authored-by: Yuya Ebihara <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Hongyue/Steve Zhang <[email protected]>
Co-authored-by: Eduard Tudenhoefner <[email protected]>
Co-authored-by: Tom Tanaka <[email protected]>
Co-authored-by: Ryan Blue <[email protected]>
Co-authored-by: Aihua Xu <[email protected]>
Co-authored-by: Fokko Driesprong <[email protected]>
Co-authored-by: ConradJam <[email protected]>
Co-authored-by: Bryan Keller <[email protected]>
Co-authored-by: Daniel Weeks <[email protected]>
Co-authored-by: pvary <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants