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

feat(ui): add deletion prompt when deleting an active or archived recording #463

Merged
merged 18 commits into from
Jul 5, 2022

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented Jun 16, 2022

Fixes #462
Also fixes #382

I didn't add a task or mention to add a prompt for deleting rules yet because I wanted to take it steady with React. If anyone thinks it should be included in this PR and the associated issue, please feel free.

Any feedback and thoughts on what I've done?

@maxcao13 maxcao13 added the feat New feature or request label Jun 16, 2022
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Looks pretty good so far. I would suggest keeping the language within the modal generic so that the same component can be reused for various different deletion confirmations. Any usecase-specific text, like having a message about "recordings" in particular, should be passed in as props.

Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

This is looking really, really good so far. Nice work. Looking forward to getting this merged :-)

@maxcao13
Copy link
Member Author

Btw, I notice that passing clean=true does not actually delete any active recordings created by the Automated Rule as mentioned here cryostatio/cryostatio.github.io#36 (comment). Was this intended to be implemented later after this task was resolved?

@maxcao13 maxcao13 marked this pull request as ready for review June 28, 2022 17:16
@andrewazores
Copy link
Member

?clean=true is implemented: https://github.com/cryostatio/cryostat/blob/f83117582d5404a3d1590a09cbabbd67c0bea120/src/main/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandler.java#L149

It doesn't delete active recordings, it just stops them. That's intentional, so that the runtime throughput overhead of the recording is removed, but the latest JFR data is still available for the user to grab if they need it.

@maxcao13
Copy link
Member Author

I see, got it thanks ^^

Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Nice, I like it. Seems to work quite well.

https://www.patternfly.org/v4/components/modal/design-guidelines#confirm-a-destructive-action

image

vs

image

Going by the Patternfly design guidelines for modals like these, I think we can drop the "This cannot be undone" if we just add "Permanently" to the modal's title. There should be a brief description of what data is lost when an active recording is deleted, too.

@maxcao13
Copy link
Member Author

Unrelated, but how much does the "react-axe" dependency matter for our development? It seems to popup a lot for me in the web developer console in my browser detailing accessibility issues, but I don't see much fuss about it, just wondering.

@andrewazores
Copy link
Member

Accessibility is important and something we should paid more attention to. Internationalization/localization, too.

maxcao13 added 11 commits June 30, 2022 18:45
… doing the clean query parameter for automated rules
…to implement this on the AllArchivesView, will do tests after and fix UI for AutomatedRule dialog where the two checkboxes look weird together
… console.logs around, and added a second part to the description which details what info will be lost upon deletion
…re useCallback and refactored many inline anonymous functions out
…ved items from modals and put labels in the props
@andrewazores
Copy link
Member

Sorry, I used a code formatter on VSCode and it touched a lot of random places like whitespace.

No worries. It's best to contain that to a single commit if and when practical, but sometimes this happens. FWIW there's also npm run format that you can do from your shell.

@maxcao13
Copy link
Member Author

maxcao13 commented Jul 4, 2022

Sorry, I used a code formatter on VSCode and it touched a lot of random places like whitespace.

No worries. It's best to contain that to a single commit if and when practical, but sometimes this happens. FWIW there's also npm run format that you can do from your shell.

When I run npm run format it doesn't detect and fix things that my VSCode extension does, for example misaligned tabs or extra whitespace. Is there some sort of config file that @magic/format uses? I notice that there is a .prettierrc, but I'm not sure if that's it. I also notice that the format script in package.json runs

"prettier --check --write ./src/**/*.{tsx,ts} && license-check-and-add add -f license-config.json"

but I don't what config it uses. Sorry this may be better as a question on a separate issue.

@andrewazores
Copy link
Member

Sorry, I used a code formatter on VSCode and it touched a lot of random places like whitespace.

No worries. It's best to contain that to a single commit if and when practical, but sometimes this happens. FWIW there's also npm run format that you can do from your shell.

When I run npm run format it doesn't detect and fix things that my VSCode extension does, for example misaligned tabs or extra whitespace. Is there some sort of config file that @magic/format uses? I notice that there is a .prettierrc, but I'm not sure if that's it. I also notice that the format script in package.json runs

"prettier --check --write ./src/**/*.{tsx,ts} && license-check-and-add add -f license-config.json"

but I don't what config it uses. Sorry this may be better as a question on a separate issue.

What is @magic/format?

prettier uses the .prettierrc, that's one of the defaults it looks for and is a common config file convention.

$ npx prettier --help
Usage: prettier [options] [file/dir/glob ...]

By default, output is written to stdout.
Stdin is read if it is piped to Prettier and no files are given.

Output options:

  -c, --check              Check if the given files are formatted, print a human-friendly summary
                           message and paths to unformatted files (see also --list-different).
  -l, --list-different     Print the names of files that are different from Prettier's formatting (see also --check).
  -w, --write              Edit files in-place. (Beware!)

Format options:

  --arrow-parens <always|avoid>
                           Include parentheses around a sole arrow function parameter.
                           Defaults to always.
  --bracket-same-line      Put > of opening tags on the last line instead of on a new line.
                           Defaults to false.
  --no-bracket-spacing     Do not print spaces between brackets.
  --embedded-language-formatting <auto|off>
                           Control how Prettier formats quoted code embedded in the file.
                           Defaults to auto.
  --end-of-line <lf|crlf|cr|auto>
                           Which end of line characters to apply.
                           Defaults to lf.
  --html-whitespace-sensitivity <css|strict|ignore>
                           How to handle whitespaces in HTML.
                           Defaults to css.
  --jsx-single-quote       Use single quotes in JSX.
                           Defaults to false.
  --parser <flow|babel|babel-flow|babel-ts|typescript|espree|meriyah|css|less|scss|json|json5|json-stringify|graphql|markdown|mdx|vue|yaml|glimmer|html|angular|lwc>
                           Which parser to use.
  --print-width <int>      The line length where Prettier will try wrap.
                           Defaults to 80.
  --prose-wrap <always|never|preserve>
                           How to wrap prose.
                           Defaults to preserve.
  --quote-props <as-needed|consistent|preserve>
                           Change when properties in objects are quoted.
                           Defaults to as-needed.
  --no-semi                Do not print semicolons, except at the beginning of lines which may need them.
  --single-quote           Use single quotes instead of double quotes.
                           Defaults to false.
  --tab-width <int>        Number of spaces per indentation level.
                           Defaults to 2.
  --trailing-comma <es5|none|all>
                           Print trailing commas wherever possible when multi-line.
                           Defaults to es5.
  --use-tabs               Indent with tabs instead of spaces.
                           Defaults to false.
  --vue-indent-script-and-style
                           Indent script and style tags in Vue files.
                           Defaults to false.

Config options:

  --config <path>          Path to a Prettier configuration file (.prettierrc, package.json, prettier.config.js).
  --no-config              Do not look for a configuration file.
  --config-precedence <cli-override|file-override|prefer-file>
                           Define in which order config files and CLI options should be evaluated.
                           Defaults to cli-override.
  --no-editorconfig        Don't take .editorconfig into account when parsing configuration.
  --find-config-path <path>
                           Find and print the path to a configuration file for the given input file.
  --ignore-path <path>     Path to a file with patterns describing files to ignore.
                           Defaults to .prettierignore.
  --plugin <path>          Add a plugin. Multiple plugins can be passed as separate `--plugin`s.
                           Defaults to [].
  --plugin-search-dir <path>
                           Custom directory that contains prettier plugins in node_modules subdirectory.
                           Overrides default behavior when plugins are searched relatively to the location of Prettier.
                           Multiple values are accepted.
                           Defaults to [].
  --with-node-modules      Process files inside 'node_modules' directory.

Editor options:

  --cursor-offset <int>    Print (to stderr) where a cursor at the given position would move to after formatting.
                           This option cannot be used with --range-start and --range-end.
                           Defaults to -1.
  --range-end <int>        Format code ending at a given character offset (exclusive).
                           The range will extend forwards to the end of the selected statement.
                           This option cannot be used with --cursor-offset.
                           Defaults to Infinity.
  --range-start <int>      Format code starting at a given character offset.
                           The range will extend backwards to the start of the first line containing the selected statement.
                           This option cannot be used with --cursor-offset.
                           Defaults to 0.

Other options:

  --no-color               Do not colorize error messages.
  --no-error-on-unmatched-pattern
                           Prevent errors when pattern is unmatched.
  --file-info <path>       Extract the following info (as JSON) for a given file path. Reported fields:
                           * ignored (boolean) - true if file path is filtered by --ignore-path
                           * inferredParser (string | null) - name of parser inferred from file path
  -h, --help <flag>        Show CLI usage, or details about the given flag.
                           Example: --help write
  -u, --ignore-unknown     Ignore unknown files.
  --insert-pragma          Insert @format pragma into file's first docblock comment.
                           Defaults to false.
  --loglevel <silent|error|warn|log|debug>
                           What level of logs to report.
                           Defaults to log.
  --require-pragma         Require either '@prettier' or '@format' to be present in the file's first docblock comment
                           in order for it to be formatted.
                           Defaults to false.
  --stdin-filepath <path>  Path to the file to pretend that stdin comes from.
  --support-info           Print support information as JSON.
  -v, --version            Print Prettier version.

@andrewazores
Copy link
Member

prettier, and other things, fixed up in #478.

@maxcao13 would you be able to wind back and undo/redo 23e5b2b to remove your IDE's auto-formatting changes, and apply only the review fixes? It's probably easiest to do by just doing a git revert, and then re-applying the fixes manually, disabling any auto-formatting in your IDE if necessary. This makes the patch a lot easier to read and review, and it also minimizes the cross-PR rebase effect this has on other open PRs.

@maxcao13
Copy link
Member Author

maxcao13 commented Jul 5, 2022

No problem!

@maxcao13
Copy link
Member Author

maxcao13 commented Jul 5, 2022

Sorry, I used a code formatter on VSCode and it touched a lot of random places like whitespace.

No worries. It's best to contain that to a single commit if and when practical, but sometimes this happens. FWIW there's also npm run format that you can do from your shell.

When I run npm run format it doesn't detect and fix things that my VSCode extension does, for example misaligned tabs or extra whitespace. Is there some sort of config file that @magic/format uses? I notice that there is a .prettierrc, but I'm not sure if that's it. I also notice that the format script in package.json runs

"prettier --check --write ./src/**/*.{tsx,ts} && license-check-and-add add -f license-config.json"

but I don't what config it uses. Sorry this may be better as a question on a separate issue.

What is @magic/format?

Sorry was looking at the wrong dependency on npm, oops.

maxcao13 added 3 commits July 5, 2022 11:54
…ff, removed items from modals and put labels in the props"

This reverts commit 23e5b2b.
@andrewazores andrewazores merged commit 9f9a325 into cryostatio:main Jul 5, 2022
@maxcao13 maxcao13 deleted the feat branch July 5, 2022 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request needs-documentation
Projects
None yet
3 participants