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

Closes #1187: Extend the configuration UI to be able to hide files #1493

Merged
merged 9 commits into from
Aug 1, 2022

Conversation

marcel-haag
Copy link
Contributor

@marcel-haag marcel-haag commented Jul 4, 2022

closes #1187

This change is Reviewable

@marcel-haag marcel-haag requested a review from mariusoe July 4, 2022 12:50
@heiko-holz heiko-holz changed the title feat: Extend the configuration UI to be able to hide files Closes #1187: Extend the configuration UI to be able to hide files Jul 12, 2022
@heiko-holz heiko-holz self-assigned this Jul 12, 2022
Copy link
Member

@MariusBrill MariusBrill left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marcel-haag and @mariusoe)


components/inspectit-ocelot-configurationserver-ui/src/redux/ducks/configuration/actions.js line 269 at r2 (raw file):

      hideFilesRecursively(files, hidePattern);
    } else {
      dispatch(fetchFiles());

Why do we need to fetch the files when showHiddenFiles is false?

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marcel-haag and @mariusoe)

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marcel-haag and @mariusoe)

heiko-holz
heiko-holz previously approved these changes Jul 13, 2022
Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MariusBrill and @mariusoe)


components/inspectit-ocelot-configurationserver-ui/src/redux/ducks/configuration/actions.js line 269 at r2 (raw file):

Previously, MariusBrill (Marius Brill) wrote…

Why do we need to fetch the files when showHiddenFiles is false?

because we toggle from showHiddenFiles to !showHiddenFiles.

But I see your point. Doing the toggle before and then negating the expression makes more sense.

I've pushed an update.

Copy link
Member

@aaronweissler aaronweissler left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 9 files reviewed, 1 unresolved discussion (waiting on @heiko-holz, @marcel-haag, @MariusBrill, and @mariusoe)

a discussion (no related file):
When I create a new file or folder while hidden files are not shown, they all appear again. I assume it is because in the writeFile action the fetchFiles action is dispatched as well.
Maybe the hiding could be incorporated into the fetchFiles action directly?

I also wonder if the default should be that the hidden files are not shown, so they are only shown when explicitly asked for?


@heiko-holz
Copy link
Contributor

a discussion (no related file):

Previously, aaronweissler wrote…

When I create a new file or folder while hidden files are not shown, they all appear again. I assume it is because in the writeFile action the fetchFiles action is dispatched as well.
Maybe the hiding could be incorporated into the fetchFiles action directly?

I also wonder if the default should be that the hidden files are not shown, so they are only shown when explicitly asked for?

Good point, thank you for the comment!

I have refactored toggleShowHiddenFiles and incorporated it in fetchFiles.

Let's discuss the default state in the next meeting.

@heiko-holz
Copy link
Contributor

a discussion (no related file):

Previously, heiko-holz (Heiko Holz) wrote…

Good point, thank you for the comment!

I have refactored toggleShowHiddenFiles and incorporated it in fetchFiles.

Let's discuss the default state in the next meeting.

The default state was now changed to false, i.e., hidden files are not shown per default

heiko-holz
heiko-holz previously approved these changes Aug 1, 2022
Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r5, 3 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aaronweissler and @mariusoe)

aaronweissler
aaronweissler previously approved these changes Aug 1, 2022
Copy link
Member

@aaronweissler aaronweissler left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mariusoe)

@heiko-holz heiko-holz dismissed stale reviews from aaronweissler and themself via d993899 August 1, 2022 11:04
Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mariusoe)

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mariusoe)

Copy link
Member

@MariusBrill MariusBrill left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mariusoe)

@heiko-holz heiko-holz merged commit 135e0d6 into inspectIT:master Aug 1, 2022
aaronweissler pushed a commit that referenced this pull request Sep 5, 2022
…1493)

* feat: Extend the configuration UI to be able to hide files

* Refactor method and field names; move the functionality to show/hide hidden files to the FileToolbar.js

* Minor refactor of toggleShowHiddenFiles

* Fix comment

* Highlight hidden files when shown (change color)

* Fix linting errors

* refactor toggle show hidden files; move functionality to fetchFiles

* change default state of showHiddenFiles

Co-authored-by: Heiko Holz <heiko.holz@novatec-gmbh.de>
@heiko-holz heiko-holz added the enhancement New feature or request label Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend the configuration UI to be able to hide files
4 participants