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

Added Confirm Warning for Fully Enable feature #665

Merged

Conversation

Dhruv8
Copy link

@Dhruv8 Dhruv8 commented Sep 9, 2022

Added Confirm Warning when fully enabling a feature

Copy link
Collaborator

@thetimbanks thetimbanks left a comment

Choose a reason for hiding this comment

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

As mentioned in #654 having this ability behind a UI configuration would be helpful.

@@ -225,7 +225,7 @@
<div class="row">
<% unless @feature.boolean_value %>
<div class="col">
<button type="submit" name="action" value="Enable" class="btn btn-outline-success btn-block">
<button type="submit" name="action" value="Enable" id="confirm-warn-enable" class="btn btn-outline-success btn-block">
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to make this id a little more generic. Something like enable_feature__button. That would stay consistent with some of the other id attributes on buttons.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @thetimbanks changed the id as requested, it truly makes sense! Can i please get it merged, i believe it would be of great help for our product, if this small change could merge.

Copy link
Collaborator

@thetimbanks thetimbanks Sep 13, 2022

Choose a reason for hiding this comment

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

@Dhruv8 Would you be able to put this behind a configuration with it disabled by default? This is a feature that not everyone will want so it would be best to have a configuration to control when it is enabled.

This was brought up previously in this issue: #654

Copy link
Author

Choose a reason for hiding this comment

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

Hi @thetimbanks Added the configuration option and it is set to false by default.

Copy link
Author

Choose a reason for hiding this comment

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

Reminder @thetimbanks for merging the PR, cc @rsanheim @bkeepers @delynn

@jnunemaker
Copy link
Collaborator

jnunemaker commented Sep 28, 2022 via email

@jnunemaker jnunemaker merged commit 95d8434 into flippercloud:master Sep 28, 2022
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.

3 participants