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

Time-boxed entries for the banner #4429

Merged
merged 60 commits into from
May 16, 2024

Conversation

ricardoamaro
Copy link
Contributor

@ricardoamaro ricardoamaro commented May 6, 2024

Dynamically manage and display banner entries as suggested in #4380

  1. Store the banner details (including end dates) in a the _index.md file.
  2. Use templating in banners.md file to sort the entries by the end dates (.to:)
  3. Filter out those that have expired

@ricardoamaro ricardoamaro requested a review from a team May 6, 2024 19:38
@ricardoamaro
Copy link
Contributor Author

ricardoamaro commented May 6, 2024

It should be good. Here is a test I did with some debug:
https://deploy-preview-4429--opentelemetry.netlify.app/
image

Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

Thanks! Looks like it works as expected, a few additional suggestions

@svrnm svrnm requested a review from a team May 7, 2024 08:11
ricardoamaro and others added 2 commits May 7, 2024 16:28
Co-authored-by: Severin Neumann <[email protected]>
Co-authored-by: Severin Neumann <[email protected]>
@svrnm
Copy link
Member

svrnm commented May 10, 2024

Please also let me know if we still should limit to 2 entries.

Yes, we might consider that sometimes we want to have more than 2, but this should be the default, otherwise this is in a really good shape, thx.

@chalin chalin self-assigned this May 11, 2024
@ricardoamaro
Copy link
Contributor Author

/fix:all

@opentelemetrybot
Copy link
Collaborator

You triggered fix:all action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/9046659360

@opentelemetrybot
Copy link
Collaborator

fix:all run failed, please check https://github.com/open-telemetry/opentelemetry.io/actions/runs/9046659360 for details

@ricardoamaro
Copy link
Contributor Author

/fix:all

@opentelemetrybot
Copy link
Collaborator

You triggered fix:all action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/9046740195

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @ricardoamaro. See inline comments.

@svrnm - even after the suggested edits are addressed, this should be put on hold until we get an answer to #2402 (comment).

@ricardoamaro
Copy link
Contributor Author

/fix:all

@opentelemetrybot
Copy link
Collaborator

You triggered fix:all action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/9086168074

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Thanks for making the suggested changes. LGTM

@svrnm
Copy link
Member

svrnm commented May 16, 2024

hey @ricardoamaro, apologies for taking that long, but when we started this I didn't anticipate that we would make a move towards internationalization & localization, which as @chalin mentioned in #2402 (comment) requires a different approach eventually. We want to appreciate your contribution, so I am going to merge the PR in, but I wanted to be fully transparent, that this change will see some major rework again. Thank you!

@svrnm svrnm merged commit 5e09ac0 into open-telemetry:main May 16, 2024
15 checks passed
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.

5 participants