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

frontend: Show feature flags in topbar and separate page #1144

Merged
merged 4 commits into from
Nov 13, 2020

Conversation

almusil
Copy link
Contributor

@almusil almusil commented Oct 30, 2020

As discussed in #1129. Opening the frontend part for discussion.

Current implementation look:
Feature flags

@jyn514 jyn514 added the A-frontend Area: Web frontend label Oct 30, 2020
@pietroalbini
Copy link
Member

I'm not sure showing the additional dependencies is much useful, we could simplify the layout like this (showing the features on two columns on desktop):

photo_2020-10-30_11-41-39

@almusil
Copy link
Contributor Author

almusil commented Oct 30, 2020

I'm not sure showing the additional dependencies is much useful, we could simplify the layout like this (showing the features on > two columns on desktop):

From my point of view it's useful to know which feature gets enabled by which flag. Especially for default.

On the other hand it would be useful to have at least something to show and the detail can be added later on.

@jyn514 jyn514 added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Oct 30, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 30, 2020

I also prefer having the features that are enabled shown.

@Kixiron
Copy link
Member

Kixiron commented Oct 30, 2020

I think we should show full detail, but on a separate page (placed alongside /builds) with the top bar linking to it

@Kixiron
Copy link
Member

Kixiron commented Oct 30, 2020

Made a very rough and out of proportion sketch of what I'm thinking, display the available features on the left, clicking on each "maximizes" it to show all the features and dependencies it activates. This allows us to give as much info as possible (ex. saying if the feature is actually a feature or just an optional dependency) without immediately overwhelming the user.

image

@jyn514 jyn514 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 Nov 4, 2020
@almusil
Copy link
Contributor Author

almusil commented Nov 5, 2020

Ok, I have been able to come up with this (I'm really bad at frontend, so please excuse some rough edges):
Screenshot_2020-11-05 regex 1 3 1 - Docs rs
Screenshot_2020-11-05 regex 1 3 1 - Docs rs(1)
Screenshot_2020-11-05 regex 1 3 1 - Docs rs(2)

@jyn514
Copy link
Member

jyn514 commented Nov 5, 2020

I like it! It could be nice to split it into columns like @pietroalbini suggested, but I don't think we need to block on that.

@jyn514 jyn514 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 Nov 5, 2020
@almusil almusil changed the title WIP: frontend: Show feature flags in topbar frontend: Show feature flags in topbar and separate page Nov 9, 2020
Copy link
Member

@jyn514 jyn514 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 sticking with this :)

src/db/types.rs Outdated Show resolved Hide resolved
src/web/crate_details.rs Outdated Show resolved Hide resolved
templates/crate/features.html Outdated Show resolved Hide resolved
templates/rustdoc/topbar.html Outdated Show resolved Hide resolved
templates/crate/features.html Outdated Show resolved Hide resolved
src/web/crate_details.rs Outdated Show resolved Hide resolved
src/web/crate_details.rs Outdated Show resolved Hide resolved
src/web/features.rs Outdated Show resolved Hide resolved
So far features were stored only in database.
Add link to the topbar which will lead to the
new features page. Features page will show
all relevant features with their subfeatures.
@jyn514
Copy link
Member

jyn514 commented Nov 10, 2020

@almusil it's not a big deal, but in the future it would be nice to add changes as additional commits instead of force-pushing. The diff github shows also includes the changes from master. Fortunately here there weren't many changes, but in a big repo like rust-lang/rust it would be unreadable.

(FWIW I consider this a bug in github isaacs/github#1834)

@almusil
Copy link
Contributor Author

almusil commented Nov 10, 2020

@almusil it's not a big deal, but in the future it would be nice to add changes as additional commits instead of force-pushing. The diff github shows also includes the changes from master. Fortunately here there weren't many changes, but in a big repo like rust-lang/rust it would be unreadable.

(FWIW I consider this a bug in github isaacs/github#1834)

Ah right, the ugly thing about this is that every repo has different policy. I'll keep it in mind for future changes in this repo, thanks :)

src/web/crate_details.rs Outdated Show resolved Hide resolved
src/test/fakes.rs Outdated Show resolved Hide resolved
templates/crate/features.html Outdated Show resolved Hide resolved
templates/header/package_navigation.html Show resolved Hide resolved
templates/rustdoc/topbar.html Show resolved Hide resolved
Show different page for empty feature flags and
null feature flags.
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This already looks really good and I'm happy to merge it as is :) Some improvements I'd like to see at some point:

  • Right now the features look like they're unordered:
    image
    Could we change it to put features that enable other features first? Or we could get really fancy and do a tree structure, where we put default -- [std, perf, unicode], then std, then perf, then unicode, then, recursively, features that are enabled by std/perf/unicode.
  • We don't distinguish between optional dependencies and features. Could we add that?
  • @Nemo157 I think you mentioned a while back you wouldn't want this for your crates - do you still want to add an opt-out to package.metadata.docs.rs?
  • Could we add some color? Maybe green for default, red for features that enable other features, black otherwise, something like that?

src/web/crate_details.rs Outdated Show resolved Hide resolved
@Nemo157
Copy link
Member

Nemo157 commented Nov 10, 2020

  • @Nemo157 I think you mentioned a while back you wouldn't want this for your crates - do you still want to add an opt-out to package.metadata.docs.rs?

I would, but I wouldn't want to block this on that being added.

I did run across a bug in the feature recording while looking at it though, async-compression 0.3.6 has a tokio-02 and a tokio-03 optional dependency, but these both get recorded and displayed as tokio.

{%- if metadata.features -%}
<p>This version has <b>{{ metadata.features | length }}</b> feature flags, <b data-id="default-feature-len">
{%- if metadata.features[0].name == 'default' -%}
{{ metadata.features[0].subfeatures | length }}
Copy link
Member

Choose a reason for hiding this comment

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

This count is wrong when one of the default features activates more features. In bs58 0.4.0 I have default = ["std"] and std = ["alloc"].

Also I think the previous count should be decreased by 1 (if the crate has a default feature), otherwise a crate which activates all its features by default will still say something like "This version has 5 feature flags, 4 of them enabled by default" because default is not counted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

async-compression 0.3.6

I was trying it locally and I can see both tokio-02 and tokio-03.

Copy link
Member

Choose a reason for hiding this comment

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

Since the renamed dependency issue is unrelated to this UI I opened #1175 for it.

@almusil
Copy link
Contributor Author

almusil commented Nov 11, 2020

@jyn514 @Nemo157 What should be addressed by this PR? The default counting to follow the dependencies?

@jyn514
Copy link
Member

jyn514 commented Nov 11, 2020

@almusil if you can fix the bugs Nemo noticed and address my nit I think this is good to merge. The rest can be fixed in follow-ups.

@jyn514 jyn514 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 Nov 11, 2020
@almusil
Copy link
Contributor Author

almusil commented Nov 11, 2020

@jyn514 Hopefully got it right this time, thanks for all the patience

EDIT: We should also reference #590 as this is addressing that. (I don't know if it can be closed after this merge).

Order features that are enabled by default in flat
tree structure. At the same time report correct
number of features being enabled by default.
src/web/features.rs Outdated Show resolved Hide resolved
src/web/features.rs Outdated Show resolved Hide resolved
Comment on lines +47 to +49
let result = order_features_and_count_default_len(raw);
features = Some(result.0);
default_len = result.1;
Copy link
Member

Choose a reason for hiding this comment

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

rust-lang/rust#71156 would help with this :) 'coming soon to rust versions near you'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will be nice :)

src/web/features.rs Outdated Show resolved Hide resolved
By making it more modular it is easier to add
unit tests fro current behavior.
@jyn514
Copy link
Member

jyn514 commented Nov 13, 2020

I think further improvements can be done in follow-ups. Thanks so much for working on this, I've wanted this in docs.rs for a while ❤️

@jyn514 jyn514 merged commit 6a81b64 into rust-lang:master Nov 13, 2020
@almusil
Copy link
Contributor Author

almusil commented Nov 15, 2020

Happy to help :) Thank you for your patience. If you don't mind I would like to continue with improvements to this feature.

@jyn514
Copy link
Member

jyn514 commented Nov 15, 2020

Sounds great! If you want to talk about ideas we have a discord channel for docs.rs

@almusil almusil deleted the feature_frontend branch November 16, 2020 08:26
apiraino added a commit to apiraino/docs.rs that referenced this pull request Mar 17, 2021
This patch changes one detail implemented in rust-lang#1144. The features flag
sorting was by "number of subfeatures". Now it by feature alphabetically.
It also ensure that the "default" feature stays on top of the list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: Web frontend S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants