Skip to content

Conversation

@tomzohar
Copy link
Contributor

@tomzohar tomzohar commented Mar 5, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: CONTRIBUTING.md
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?
add file-system navigator

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #5

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

image

@nx-cloud
Copy link

nx-cloud bot commented Mar 5, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 61a9dd9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 3 targets

Sent with 💌 from NxCloud.

Copy link
Contributor

@OmerGronich OmerGronich left a comment

Choose a reason for hiding this comment

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

please move filesystem-navigator and toolbar to workspace-manager/src/lib/ui instead of features

and also filesystem-navigator-utils should either be deleted or at the very least moved to data-access, and the word "utils" should be removed from the name

@tomzohar tomzohar requested a review from OmerGronich March 5, 2023 12:09
@ronnetzer
Copy link
Member

regarding the visuals of it, the toolbar should look like a filesystem path, you should change the> separator to per platform separator.
also note the angular icon looks like its in low resolution, also its not aligned with the text. (personally I think it should replace the directory icon or it should be moved to the end of the row [right side])

@tomzohar
Copy link
Contributor Author

tomzohar commented Mar 7, 2023

regarding the visuals of it, the toolbar should look like a filesystem path, you should change the> separator to per platform separator. also note the angular icon looks like its in low resolution, also its not aligned with the text. (personally I think it should replace the directory icon or it should be moved to the end of the row [right side])

@OmerGronich WDYT about replacing the folder icon with the angular logo instead of adding the angular logo at the end ?
image

@OmerGronich
Copy link
Contributor

regarding the visuals of it, the toolbar should look like a filesystem path, you should change the> separator to per platform separator. also note the angular icon looks like its in low resolution, also its not aligned with the text. (personally I think it should replace the directory icon or it should be moved to the end of the row [right side])

@OmerGronich WDYT about replacing the folder icon with the angular logo instead of adding the angular logo at the end ? image

Not bad IMO, @Danieliverant what do you think?

@Danieliverant
Copy link
Collaborator

regarding the visuals of it, the toolbar should look like a filesystem path, you should change the> separator to per platform separator. also note the angular icon looks like its in low resolution, also its not aligned with the text. (personally I think it should replace the directory icon or it should be moved to the end of the row [right side])

@OmerGronich WDYT about replacing the folder icon with the angular logo instead of adding the angular logo at the end ? image

Not bad IMO, @Danieliverant what do you think?

I really like it

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #93 (61a9dd9) into master (a2ea9b6) will increase coverage by 2.45%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
+ Coverage   61.88%   64.34%   +2.45%     
==========================================
  Files          72       77       +5     
  Lines         551      589      +38     
  Branches       27       27              
==========================================
+ Hits          341      379      +38     
  Misses        209      209              
  Partials        1        1              
Flag Coverage Δ *Carryforward flag
cli-daemon 49.86% <ø> (ø)
cli-gui 89.83% <ø> (ø)
configuration 100.00% <ø> (ø) Carriedforward from a2ea9b6
executors 100.00% <ø> (ø) Carriedforward from a2ea9b6
generators 75.75% <ø> (ø)
shared ∅ <ø> (∅) Carriedforward from a2ea9b6
ui 100.00% <ø> (ø) Carriedforward from a2ea9b6
workspace-manager 92.68% <100.00%> (+6.31%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
...c/lib/data-access/workspace-manager-api.service.ts 100.00% <100.00%> (ø)
...oolbar/filesystem-navigator-toolbar.component.html 100.00% <100.00%> (ø)
...-toolbar/filesystem-navigator-toolbar.component.ts 100.00% <100.00%> (ø)
...stem-navigator/filesystem-navigator.component.html 100.00% <100.00%> (ø)
...system-navigator/filesystem-navigator.component.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tomzohar tomzohar requested a review from a team March 7, 2023 15:52
@tomzohar tomzohar requested a review from Danieliverant March 8, 2023 12:24
@tomzohar tomzohar merged commit 8774af7 into master Mar 19, 2023
@tomzohar tomzohar deleted the tom/filesystem_navigator branch March 19, 2023 14:14
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.

6 participants