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

Swap the flyout version to be a code-branch icon #124

Merged
merged 5 commits into from
Feb 14, 2024
Merged

Conversation

ericholscher
Copy link
Member

This will look cleaner and more obvious to users than the v: that it means a version.
I couldn't get the actual fontawesome icon to load,
but wanted to open this PR as a starting point.

If we're not loading Fontawesome icons via CDN,
which makes sense for performance,
we can perhaps load it via svg?

This will look cleaner and more obvious to users than the `v:` that it means a version.
I couldn't get the actual fontawesome icon to load,
but wanted to open this PR as a starting point.

If we're not loading Fontawesome icons via CDN,
which makes sense for performance,
we can perhaps load it via svg?
@humitos
Copy link
Member

humitos commented Sep 13, 2023

This will look cleaner and more obvious to users than the v: that it means a version. I couldn't get the actual fontawesome icon to load, but wanted to open this PR as a starting point.

We talked a little about this on Slack and we weren't sure about start making changes to the UI element yet. I opened #97 but I didn't implemented yet because of the same reason.

If we're not loading Fontawesome icons via CDN, which makes sense for performance, we can perhaps load it via svg?

We are not using any external resource. Everything is self-contained/bundled into the readthedocs-addons.js we inject at serve time via CF. The pattern we are using here is a little different from the regular fa- CSS classes. If you want to use an icon, you have to import the icon using js import and then render it on the HTML part where you want to display it:

// Import the required functions and icons
import { library, icon } from "@fortawesome/fontawesome-svg-core";
import { faBinoculars } from "@fortawesome/free-solid-svg-icons";

// Add the icon to the library
library.add(faBinoculars);

// Instantiate the icon
const binoculars = icon(faBinoculars)

// Render the icon inside the HTML code
return = html`
   <div>
     ${binoculars.node[0]}
     <p class="title">Some text.</p>
   </div>`

BTW, welcome to the JavaScript world! 🥴

@agjohnson
Copy link
Contributor

Noted on #97 as well, but I feel like what we're aiming for with the first implementation of this flyout is something that users could easily reuse over the flyout menu we current give to projects. So, for now, I'm hesitant to iterate much at all on the styles and structure, and instead collect these changes in a separate implementation entirely,

@ericholscher
Copy link
Member Author

ericholscher commented Sep 13, 2023

Cool, I'm happy to hold off on this, just wanted to do a small contribution.

I don't think it's a big risk to change the beta flyout, and I'm not sure what the timeline is for a larger refactor, so small improvements seem useful to me 🤷 Overall, I don't think we want people changing the flyout itself, but instead incorporating the data into the built docs, I thought?

@humitos
Copy link
Member

humitos commented Sep 14, 2023

Overall, I don't think we want people changing the flyout itself, but instead incorporating the data into the built docs, I thought?

Yes, I'm on the same boat. Every time I think about this, I lean more towards the idea of "use our own integrations as-is, or create your own using the JSON data". I feel a lot more comfortable (myself and as a company) to compromise ourselves to maintain an API structure over time than a HTML/CSS/JS one.

I did an experiment "using the role of a Sphinx theme author" and created the flyout integration in our own theme at readthedocs/sphinx_rtd_theme#1526. I felt great about it. It was a clean pattern, simple to do, easy to integrate and looks great. That showed me that "using the JSON data" is pretty simple for users and give them a lot of power while reducing our maintenance burden.

If we ever want to allow users to "customize our own addons", I'd say it should be via the WebUI/dashboard using specific options that we control. I feel that giving users more control than this for customization of our addons it's gonna be a nightmare in the future.

@agjohnson
Copy link
Contributor

I'd say it should be via the WebUI/dashboard using specific options that we control

💯 Absolutely! This would be a killer feature of a new flyout, I think we've long wanted something like this. There are plenty of options would could surface if we wanted: "Don't show links to the dashboard" or "Flyout background color: #445", etc.

I do agree that heavy customizations should be a theme/author change though, so there is definitely some threshold that we shouldn't cross with our own customizations. But anything we can do to keep authors from customizing our flyout with CSS would be a win I think.

@agjohnson
Copy link
Contributor

Overall, I don't think we want people changing the flyout itself, but instead incorporating the data into the built docs, I thought?

Well, sort of, but yes. We don't want users customizing the flyout with CSS. CSS puts the customizations out of our control. What Manuel described would be the best target I think -- most projects use the stock flyout, a UI customized flyout is for intermediate use, and full customization is for advanced users and theme maintainers.

For now, users only really have CSS for customizations. Instead of iterating on what we have, and users kicking their CSS hacks forward onto the new implementation, I'd like to start fresh with a new implementation entirely. This would give us a clean place to start from, and we should aim for some basic level of configuration from the start with this flyout -- just at the HTML level would be fine, application UI can come later.

We can definitely be playing around with ideas for the new flyout too, maybe just with a subclass of the flyout addon.

@humitos
Copy link
Member

humitos commented Sep 18, 2023

I updated @ericholscher PR to reflect the changes he wanted to do here. It looks like this now ✨

Screenshot_2023-09-18_14-30-13

Copy link
Member Author

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Much nicer than the v:

@ericholscher
Copy link
Member Author

I keep wanting this, if it seems simple to merge in the latest version.

@humitos
Copy link
Member

humitos commented Feb 14, 2024

I'm 👍🏼 on merging this PR. I don't think there is anything blocking us and it looks better. I updated this PR with all the changes from origin/main.

@ericholscher ericholscher merged commit 07e519e into main Feb 14, 2024
4 checks passed
@ericholscher ericholscher deleted the flyout-icon branch February 14, 2024 17:43
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.

None yet

3 participants