Will/lockfile explorer step3 #3741
Conversation
william2958
requested review from
iclanton,
octogonz,
apostolisms and
D4N14L
as code owners
apps/lockfile-explorer-web/src/containers/LockfileEntryDetailsView/index.tsx
Show resolved
Hide resolved
| <div className={styles.Title2}> | ||
| <img className={styles.Image} src={require('./lockfile-explorer-title-2.svg')} /> | ||
| </div> | ||
| <div className={styles.Detail}>{appPackageVersion} rushstack.io</div> |
There was a problem hiding this comment.
The old LogoPanel looked like this:
In this PR, the layout is changed to be:
The 0.0.3 rushstack.io seems a bit confusing, since 0.0.3 is the version of Lockfile Explorer, not Rush Stack.
Also in the original design, I had envisioned that clicking on the words/icon would take you to the NPM page (or https://lfx.rushstack.io ?), whereas clicking on rushstack.io would take you to the other website https://rushstack.io
What do you think? Another consideration is that the Projects/Packages box is the centerpiece of the UI, so we want to provide as much vertical scroll area as possible.
There was a problem hiding this comment.
That makes sense, I've moved the version next to the lockfile explorer text logo to make it a bit more clear, also I moved the logo to above the bookmarks sidebar as that one is less important than the left panel. The appropriate links have also been added. I moved it away from having it's own column because i felt that it was taking up a lot of horizontal real-estate for a pretty small logo, this way I think gives the app overall more room:
common/changes/@rushstack/lockfile-explorer/will-lockfile-explorer-step3_2022-11-11-20-39.json
Outdated
Show resolved
Hide resolved
apps/lockfile-explorer-web/src/containers/LockfileViewer/index.tsx
Outdated
Show resolved
Hide resolved
| <div className={styles.LockfileFilterBar}> | ||
| {entryStack.length > 1 ? <button onClick={pop}>back</button> : null} | ||
| <h5>filter:</h5> | ||
| <h5>Filter:</h5> |
There was a problem hiding this comment.
Conventionally, tab pages are meant to be like pages of a notebook, each containing its own content & state. So it's slightly unusual that the Filter: _____ (and whatever text that you typed in there) is unaffected when switching between tabs. It might make more sense for the Filter box and back/forward buttons to appear above the tab, to make it more clear that this state affects both the "Projects" and "Packages" tabs. In the same way, the filter checkboxes at the bottom could be moved outside the tab. (When they are inside the tab, it seems to imply that there are distinct checkbox states for Projects and Packages, which is not the case, in the current implementation at least.)
Let me know what you think.
There was a problem hiding this comment.
Yeah I think that makes sense for the Filter to be applicable to each tab individually, so I changed it so that it saves and uses different filter text depending on which tab your in, so they can search for things independently.
As for the checkboxes, I feel that it would make sense that they affect both tabs as the user would probably want to look for the same "hits" across both tabs, but to make it a bit more clear that it's a separate entity I've added a border to separate the list and the filter checkbox area, let me know what you think:
|
|
||
| export const displaySpecChanges = (specChanges: Map<string, ISpecChange>, dep: string): string => { | ||
| switch (specChanges.get(dep)?.type) { | ||
| case 'add': |
There was a problem hiding this comment.
Also, it's possible for the diff to be scrolled out of view, making it not obvious that there is any diff for this entry:
Maybe we could provide a visual indicator, for example show * package spec instead of package spec if anything was modified? Maybe we could even show a * in the listview as well:
I think these transformations are fairly important information when studying lockfiles, because they are the customizations that we have to fiddle with to fix problems.
There was a problem hiding this comment.
Added an asterisk to the "package spec *" header tab to indicate that there is a diff for the entry, I don't think we should show an asterisk in the list view because we need to load the package's package.json to be able to calculate if there is a spec change, (currently we just load the selected entry's package.json & calculate the spec changes then)
apps/lockfile-explorer-web/src/containers/SelectedEntryPreview/styles.scss
Show resolved
Hide resolved
Summary
This MR adds the following changes:
Quality of life improvements
Fixes
UI Changes
Details
How it was tested