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

Recreating flyout as a webcomponent #86

Merged
merged 18 commits into from
Aug 29, 2023
Merged

Recreating flyout as a webcomponent #86

merged 18 commits into from
Aug 29, 2023

Conversation

zanderle
Copy link
Collaborator

@zanderle zanderle commented Aug 2, 2023

Building on top of the work started in #66

Closes #70

@humitos
Copy link
Member

humitos commented Aug 21, 2023

I did a quick QA locally and it's looking great! 💯 . I'm super excited about this work and I'd like to deploy it soon and start testing it on production 👍🏼 . @zanderle what's the status of it? Is it ready for review? If not, what are you considering it's missing currently?

humitos added a commit to readthedocs/readthedocs.org that referenced this pull request Aug 21, 2023
Return `addons.flyout.translations` and `addons.flyout.downloads`.
Remove `addons.flyout.vcs` for now because we don't have a reliable way to
implement it without injecting data into the build which we don't want to do
anymore.

Related readthedocs/addons#86
@zanderle
Copy link
Collaborator Author

zanderle commented Aug 24, 2023

@humitos yeah, the first version is ready. One thing I am unsure about is the fact that we're grabbing the versions from config.addons.flyout.versions but we're showing the current version from config.versions.current. This might be ok, depending on how the endpoint for this will work, but we want to make sure we have a single source of truth here.

Additionally, this is important so we can mark the current version in the versions list. We could compare with config.versions.current or add a current flag in one of the versions in config.addons.flyout.versions.

Other than that, this is still a v1 of the flyout. I'm wondering if it's ok that the input to the webcomponent is the "global" config (with all the addons, and everything", or if it should be just the config.addons.flyout? But that's not necessarily a question for this PR.

@humitos
Copy link
Member

humitos commented Aug 24, 2023

@zanderle good points raised here!

Originally, I thought that the whole config will be the input for each of the addons. This is because "the current version" for example, is something that more than one addon will require. So, I moved those "shared data" into an upper level and keep the data that is only required for the particular addon under config.addons.*.

Besdies, the config.addons.flyout.versions may be something different than config.addons.search.versions because they require different attributes or the whole filter for the versions is different. However, the confg.versions.current is always the same for all the addons.

@humitos humitos marked this pull request as ready for review August 24, 2023 13:33
@humitos humitos requested review from humitos and a team as code owners August 24, 2023 13:33
@humitos humitos requested a review from agjohnson August 24, 2023 13:33
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This is great! I'm love with this work 😍

I tested it locally with different doctools and it works great!

Peek 2023-08-28 15-34

@humitos
Copy link
Member

humitos commented Aug 28, 2023

I added some small commits with simple tweaks since I want to deploy this tomorrow, together with readthedocs/readthedocs.org#10650

If there are extra changes here, we can open new PRs.

@humitos
Copy link
Member

humitos commented Aug 29, 2023

I'm gonna merge this since I want to deploy it today to start testing it.

@humitos humitos merged commit 4e6b305 into main Aug 29, 2023
@humitos humitos deleted the zanderle/newflyout branch August 29, 2023 09:27
humitos added a commit to readthedocs/readthedocs.org that referenced this pull request Aug 29, 2023
* Addons: prepare the backend for the new flyout

Return `addons.flyout.translations` and `addons.flyout.downloads`.
Remove `addons.flyout.vcs` for now because we don't have a reliable way to
implement it without injecting data into the build which we don't want to do
anymore.

Related readthedocs/addons#86

* Addons: create one feature flag per addon

This allows us to have full control over each project.

* Addons: decide whether or not enable the addon via feature flag

* Test: update response for Addons V0

* Typo

* Flyout: don't show hidden versions
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.

Flyout: convert current flyout to web component
2 participants