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

Add links to "features" page, and mark default features #2532

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Jun 24, 2024

This adds links to all the features, dependencies, and dependency features for easy navigation.

It also marks all the transitive features with a (default) marker.

I changed that so the secondary sort order is alphabetic, as that might make more sense than sorting by number of sub-features.


Fixes #2530 and #2531

This was just a quick and dirty hack during RustFest impl Days, and we were discussing some more ideas related to the Features page in general.

The Features page now looks like this, with all the links being clickable:

Bildschirmfoto 2024-07-09 um 14 27 23

@Swatinem Swatinem requested a review from a team as a code owner June 24, 2024 10:49
@github-actions github-actions bot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jun 24, 2024
@GuillaumeGomez
Copy link
Member

Unless you know what the * is about, it's not useful. Why not instead add [FEATURE] (default)?

@Swatinem Swatinem changed the title Mark default features with a * Mark default features with a (default) Jun 24, 2024
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

some first comments from the first test:

  • deffault (default) is perhaps a little much :)
  • I'm not sure if all the default markers are correct. Example: sentry-tower subfeature in sentry feateure

generally especially the top section with all default features is a little noisy, I'm not sure how this could be improved, and would merge as-is when not buggy.

@GuillaumeGomez
Copy link
Member

We still need a visual marker to make it obvious that it's not part of the feature name, hence why I suggested parens. Without parens, we then need some styling.

@syphar
Copy link
Member

syphar commented Jun 24, 2024

@GuillaumeGomez currently it looks roughly like this:

grafik

do you mean something else?

@Swatinem Swatinem marked this pull request as draft June 24, 2024 11:24
@GuillaumeGomez
Copy link
Member

The (default) sounds good to me. But I think I misunderstood what you meant in:

  • deffault (default) is perhaps a little much :)

@Swatinem
Copy link
Contributor Author

I marked this as draft as I will iterate on this a bit more.
In particular, I believe it makes sense to also tackle #2530 at the same time, as parsing the opaque subfeature into a typed enum would make a ton of sense here as well.

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jun 24, 2024
@syphar
Copy link
Member

syphar commented Jun 24, 2024

In particular, I believe it makes sense to also tackle #2530 at the same time, as parsing the opaque subfeature into a typed enum would make a ton of sense here as well.

this would be super awesome!

@Swatinem Swatinem force-pushed the mark-default-features branch from bba0a55 to 9b07432 Compare July 9, 2024 12:26
@Swatinem Swatinem changed the title Mark default features with a (default) Add links to "features" page, and mark default features Jul 9, 2024
@Swatinem
Copy link
Contributor Author

Swatinem commented Jul 9, 2024

It took a while until I got some more time to work on this.

Now this should be fairly complete with interlinks to all the own features, dependencies and features of dependencies. See the screenshot in the updated PR description.

@Swatinem Swatinem marked this pull request as ready for review July 9, 2024 12:31
@Swatinem Swatinem force-pushed the mark-default-features branch 2 times, most recently from 3e25c50 to 4919a5e Compare July 9, 2024 12:43
@Swatinem Swatinem requested a review from syphar July 10, 2024 08:40
@github-actions github-actions bot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jul 10, 2024
@syphar
Copy link
Member

syphar commented Jul 16, 2024

short update @Swatinem :

I'm sorry looking at this PR takes so long, we were finishing a bigger refactor (#2292), which will also mean that we will have to update this PR after it's merged.

Will look at the PR anyways right now

@syphar
Copy link
Member

syphar commented Jul 16, 2024

first manual test: this is beautiful :)

@syphar
Copy link
Member

syphar commented Jul 16, 2024

@Swatinem #2292 is merge now, you need to resolve some conflicts.

Will try to look into the review tomorrow or today in the evening

@Swatinem Swatinem force-pushed the mark-default-features branch 2 times, most recently from f941d6e to 317459f Compare July 17, 2024 12:42
This adds links to all the features, dependencies, and dependency features for easy navigation.

It also marks all the transitive features with a `(default)` marker.

I changed that so the secondary sort order is alphabetic, as that might make more sense than sorting by number of sub-features.
@Swatinem Swatinem force-pushed the mark-default-features branch from 317459f to 6d4a02e Compare July 17, 2024 12:47
@syphar syphar merged commit a9f2572 into rust-lang:master Jul 17, 2024
9 checks passed
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jul 17, 2024
@syphar
Copy link
Member

syphar commented Jul 17, 2024

this is really awesome :)

( I'll be offline / on a trip for next week, I'll deploy this the week after )

@Swatinem Swatinem deleted the mark-default-features branch July 17, 2024 18:40
{%- if !feature.subfeatures.is_empty() -%}
<ul class="pure-menu-list">
{%- for (name, feature) in feature.subfeatures -%}
{%- let is_default = self.is_default_feature(name) -%}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{%- let is_default = self.is_default_feature(name) -%}
{%- let is_default = is_default_feature(name) -%}

(just nitpicking ^^)

{{- dependency -}}
</a>
{% when SubFeature::DependencyFeature with {dependency, feature, optional} %}
{%- let version = self.dependency_version(dependency) -%}
Copy link
Member

Choose a reason for hiding this comment

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

Same, no need for self..

{{- feature -}}
</a>
{% when SubFeature::Dependency with (dependency) %}
{%- let version = self.dependency_version(dependency) -%}
Copy link
Member

Choose a reason for hiding this comment

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

Same here too.

@GuillaumeGomez
Copy link
Member

Gonna send a little cleanup, but it's really nitpicking. I also saw some data-id and I wonder if we use them for tests or not...

@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Jul 31, 2024
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.

Features page: link dependent features
3 participants