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

Resize and update formatting for the data dictionary page tables under Hubble #1082

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

harsha-stellar-data
Copy link

Resize data dictionary page tables and update formatting to make the responsive to different screen sizes

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@@ -200,3 +193,143 @@ select[data-testid="example-pairing-select"] {
font-family: "Font Awesome 6 Brands";
font-size: x-large;
}

/* Table Styles */
.theme-doc-markdown {
Copy link
Contributor

Choose a reason for hiding this comment

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

This css is applied throughout the whole repo and for all markdown tables (I think). And because you've hardcoded some widths it makes some tables look weird

https://developers.stellar.org/docs/validators/admin-guide/prerequisites
Screenshot 2024-11-08 at 8 29 56 PM
vs
image

https://developers.stellar.org/docs/data
image
vs
image

Copy link
Contributor

@sydneynotthecity sydneynotthecity left a comment

Choose a reason for hiding this comment

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

As @chowbao noted, I would prefer if we set a custom table style just for our data dictionaries so that we're not impacting table formatting on other pages.

My only other suggestion is to add a header to the new Table details table (contains primary keys, natural keys etc). It looks awkward to have a blank row at the top--that or remove the row entirely

Comment on lines 8 to 9
| | |
| --- | --- |
Copy link
Contributor

Choose a reason for hiding this comment

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

As an FYI, leaving the headers blank creates weird formatting on the Data Dictionaries landing page:
image

vs

image

Choose a reason for hiding this comment

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

sure @sydneynotthecity @chowbao . We have to create a new scss file and/or write custom code in the mdx files to achieve this. Also, if some other pages add tables, they may have to write custom files again instead of a global setting. So tried this approach but I agree that it does make some other tables look weird. Will make adjustments and see how the approach pans out

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Given the options, if it's too difficult to create custom code to reformat our tables, I prefer staying with the current global settings. It is better to get our tables to conform to the standard layout than to change the standard layout for all docs.

If the width of the tables decreases, you can also opt to drop 1-2 columns. At minimum we need column name, description and type. The rest are nice to have.

Choose a reason for hiding this comment

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

Removed the header info from tables to be shows on the Cards. Now only table names will be shown.

Screenshot 2024-11-14 at 3 13 37 PM

Choose a reason for hiding this comment

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

  • Made changes to address all the review comments.
  • Also, made changes to the css file to apply for,matting changes to the Dat aDictionary pages only with a custom div. Now other pages won't be impacted.

@sydneynotthecity @chowbao Please review and let me know if any further changes are needed

Choose a reason for hiding this comment

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


| | |
| --- | --- |
| Primary Key(s) | |
Copy link
Contributor

Choose a reason for hiding this comment

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

what are your thoughts on dropping primary key off of the state table dictionaries? For these tables, natural keys are used; there isn't a singular primary key

Choose a reason for hiding this comment

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

yeah. I thought of keeping the table structure same and kept it with a BLANK.
Will there be a need to create a unique key (like a generated id) for these tables instead of composite keys? If not, we can drop them completely I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's drop them completely. We have no plans to create a composite key right now

| | |
| --- | --- |
| Primary Key(s) | |
| Natural Key(s) | balance_id, asset_type, asset_code |
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:the natural keys are balance_id, ledger_entry_change and closed_at

| | |
| --- | --- |
| Primary Key(s) | |
| Natural Key(s) | contract_code_hash |
Copy link
Contributor

Choose a reason for hiding this comment

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

also needs closed_at

Choose a reason for hiding this comment

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

closed_at is present

| closed_at | The UNIX timestamp of the sequence number's age | timestamp | | | | Partition | Yes | |

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean as a natural key. The composite key for the table is contract_code_hash + closed_at

| | |
| --- | --- |
| Primary Key(s) | |
| Natural Key(s) | contract_id |
Copy link
Contributor

Choose a reason for hiding this comment

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

this is incorrect. The natural keys are ledger_key_hash and closed_at


| | |
| ------------------ | --------------------------- |
| Primary Key(s) | |
Copy link
Contributor

Choose a reason for hiding this comment

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

primary key is op_id


| | |
| --- | --- |
| Primary Key(s) | id |
Copy link
Contributor

Choose a reason for hiding this comment

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

we should keep this as sequence. I think id was something we incorrectly carried over from Horizon and doesn't exist in the XDR itself (this is a dupe of sequence)

| soroban_fee_write_1kb | Soroban write fee costs | integer | | No | |
| signature | The signing hash of the validator node which writes the transaction set to the network.This signature ensures the integrity and authenticity of the ledger, confirming that it has not been tampered with | string | | Yes | |
| node_id | The id of winning validator node which is allowed to write transaction set to the network. The winning validator is decided by the network | string | | Yes | |
| total_byte_size_of_bucket_list | | integer | | Yes | |
Copy link
Contributor

Choose a reason for hiding this comment

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

The size, in bytes, of the Bucketlist DB for the Stellar Network.

| | |
| --- | --- |
| Primary Key(s) | |
| Natural Key(s) | last_modified_ledger, ledger_entry_change |
Copy link
Contributor

Choose a reason for hiding this comment

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

missing offer_id

Choose a reason for hiding this comment

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

Offer Id is present

| offer_id | The unique identifier for this offer | integer | | | | | Yes | |

Copy link
Contributor

Choose a reason for hiding this comment

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

as part of the natural key :)

janewang and others added 7 commits November 14, 2024 14:13
* Update getEvents retention window

* Language change from default -> maximum

* trigger a new container build

---------

Co-authored-by: Molly Karcher <[email protected]>
Co-authored-by: Elliot Voris <[email protected]>
* Stellar developer docs for Galexie
* Refine CLI section based on review feedback. Add monitoring section.
* Fix order of services in Data section
* Update docs/data/galexie/admin_guide/monitoring.mdx
* Add hardware requirements section
* Remove reference to Grafana Dashboard.
@stellar-jenkins
Copy link

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.

8 participants