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

Show feature's supervisor option in inspector #119

Merged
merged 1 commit into from
Mar 21, 2021
Merged

Show feature's supervisor option in inspector #119

merged 1 commit into from
Mar 21, 2021

Conversation

AlexKrupa
Copy link
Contributor

💡 Motivation

#95

🧑‍💻 Changes

Simple approach which looks like this:
image

Notes:

  • Enum-like format (i.e. ChristmasTheme.Enabled) is easily understandable for both devs, QA and other folks.
  • Used standard padding and same text appearance as description. I played a bit with smaller (4dp) padding between feature name and supervisor but it didn't really make a difference. Also tried customizing the text appearance (e.g. Subtitle2), but in the end making it uniform with description looked best. 🤷‍♂️ Suggestions welcome though.
  • The word Supervisor could potentially be changed to something else as the nomenclature is not that obvious for non-devs. Alternatives: "dependent on", "controlled by", "controllable if", etc. What do you think?

📝 Checklist

🧪 How to test

Run the sample app. Optionally other parameters (such as description) can be added to see what it looks like.

🔮 Next steps

Either the issue can stay open or I can create a new one for improving the UI, e.g. showing supervised features in some kind of a tree hierarchy.

Copy link
Owner

@MiSikora MiSikora 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 PR. I left few comments.

Also tried customizing the text appearance (e.g. Subtitle2), but in the end making it uniform with description looked best. 🤷‍♂️ Suggestions welcome though.

It looks good to me as is.

The word Supervisor could potentially be changed to something else as the nomenclature is not that obvious for non-devs. Alternatives: "dependent on", "controlled by", "controllable if", etc. What do you think?

Leaving supervisor is ok with me but I get your point. I would avoid X if because it might be confusing when X happens to be an option on a feature. If you want to change it I would either use controlled by or maybe something like Turned on if %1$s is %2$s. The latter might look better in italics.

Either the issue can stay open or I can create a new one for improving the UI, e.g. showing supervised features in some kind of a tree hierarchy.

Issue can stay open, I'll just update it after merging this PR.

@AlexKrupa
Copy link
Contributor Author

I'll leave the string template as is.

library/docs/changelog.md Outdated Show resolved Hide resolved
MiSikora
MiSikora previously approved these changes Mar 21, 2021
Copy link
Owner

@MiSikora MiSikora left a comment

Choose a reason for hiding this comment

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

LGTM.

You'll just need to fix SyntheticAccessor Lint issue.

@MiSikora MiSikora merged commit c100852 into MiSikora:trunk Mar 21, 2021
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.

2 participants