-
Notifications
You must be signed in to change notification settings - Fork 23
feat(cellar-explorer): add objects management #1661
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
base: master
Are you sure you want to change the base?
Conversation
e16437b to
2c736be
Compare
|
🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cellar-explorer-objects-1/index.html. This preview will be deleted once this PR is closed. |
2c736be to
041c90d
Compare
041c90d to
d200f04
Compare
HeleneAmouzou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GG 👏 I have only left small feedbacks/questions, but otherwise LGTM !
| ]; | ||
|
|
||
| /** | ||
| * A component that allows to navigate through a Cellar addon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * A component that allows to navigate through a Cellar addon. | |
| * A component that allows to navigate through a Cellar addon's bucket. |
| ], | ||
| }); | ||
|
|
||
| export const fetchingObject = makeStory(conf, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question : I don't visually see the fetching state, is this story broken or am I missing something ? 🤔
|
I forgot to test on |
florian-sanders-cc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @pdesoyres-cc
I did not find big issues, the component works great and looks great as well!
I faced an issue with the pagination:
- The "next page" is available,
- I click it,
- It loads and I get empty states as if there were no objects 🤔
(the bucket I'm testing contains all my notes, it contains 430 objects)
I wonder if the pagination may confuse people: icons only with left / right arrow could also mean go back / go forward like in browsers. Like I went from a bucket to its objects, I'm looking for a "back" icon to go back to the bucket list but that's not what the left arrow button does. I think I'm exaggerating, we're fine and we can just see if we get feedback about this subject from actual users.
We need to discuss how to handle focus when navigation happens (from bucket to object, from a object level to another). Right now the focus is lost most of the time, which makes the component not keyboard friendly. I did not think this through but I think when a navigation occurs, we should move focus to the first column + first row in the grid.
Also, when deleting objects, while filtered, the focus is lost after deleting the last object filtered. It should be placed on the empty message instead but it seems that it doesn't work? 🤔
| * Dispatched when a breadcrumb item is clicked. | ||
| * @extends {CcEvent<{path: Array<string>}>} | ||
| */ | ||
| export class CcBreadcrumbClickEvent extends CcEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: tough one but I wonder if removing the "s" here is a good idea, it makes it very easy to get the event name wrong since the component name contains a "s" (but I do understand why it's singlar here 🤷)
Or we could use a totally different event name like cc-navigation-request or something like that 🤔
| } | ||
|
|
||
| ul { | ||
| align-items: end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: this breaks alignment for breadcrumbs with no icons.
To fix your issue, you can:
- set
display: inline-flex;oncc-link(this will remove the blank space below the icon that led you to choosealign-items: endin the first place), - remove this
align-items(or replaceendwithcenterbut I don't think you need it anyway 🤔)
| >${cell.value}</cc-button | ||
| >`, | ||
| template: html`<div class="icon-label"> | ||
| ${cell.icon != null ? html`<cc-icon .icon=${cell.icon} ?skeleton=${skeleton}></cc-icon>` : ''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: are we sure we'll always have decorative icons or should we expose & use a cell.iconA11yName?
| * @param {function} orElse | ||
| * @param {boolean} [deleteMode] | ||
| */ | ||
| #handleErrorOnObject(error, objectKey, orElse, deleteMode = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: 4 positioned args feels a lot to me, especially with a boolean at the end, it makes the signature harder to read & manipulate
When I read this for instance:
this.#handleErrorOnObject(
error,
objectKey,
() => notifyError(i18n('cc-cellar-object-list.error.object-deletion-failed', { objectKey })),
true,
);I have to check the actual function to understand what true means, what the callback is for.
It's a nitpick because when coding you have the LSP, which mitigates the issue but I feel like object + destructuring would be better.
| this.dispatchEvent(new CcCellarNavigateToBucketEvent(this.state.bucketName)); | ||
| } | ||
| this.dispatchEvent(new CcCellarNavigateToPathEvent(path.slice(2))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question/suggestion: I bet this doesn't actually trigger an issue but it feels weird that we don't have any early returns in there. If path.length === 1 || path.length === 2, then we're dispatching both CcCellarNavigateToHomeEvent and CcCellarNavigateToPathEvent 🤔
|
|
||
| return html` | ||
| <div class="path-wrapper"> | ||
| <cc-breadcrumbs .items=${items} @cc-breadcrumb-click=${this._onPathItemClick}></cc-breadcrumbs> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: the breadcrumb should be placed in a <nav arial-label="{some translation for breadcrumb / fil d'Ariane}"
Should the component itself wrap the list of links in <nav>? I think so 🤔 (I'm not even sure Shadow DOM surfaces region landmarks correctly but we should use a <nav> element anyway).
|
|
||
| <div class="details-sub-title">${i18n('cc-cellar-object-list.details.actions.title')}</div> | ||
| <div class="details-actions"> | ||
| <cc-button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: this should be a link unless we really don't have a choice. The current behavior does not actually trigger a download + opens a new window which is not the expected behavior.
We can discuss it in sync but I think we should stick with the way we handled this for Kube:
clever-components/src/components/cc-addon-header/cc-addon-header.smart-kubernetes.js
Line 46 in 1fbe222
| const kubeConfigFetchInterval = setInterval(() => { |
It might be too complicated to do that in your case but it's worth trying because it would allow to provide the filename + extension so that it actually downloads the file when the user clicks (right now the user has to do all of this manually).
| ${object != null | ||
| ? html`<div class="details-wrapper"> | ||
| <div class="details-icon-wrapper"> | ||
| <cc-icon .icon=${this._getFileIcon(object.contentType)}></cc-icon> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: the icon provides information about the type of file / media so we should provide an a11yName with this information
florian-sanders-cc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of feedback from Claude (I forgot to run it but figured it could catch stuff I didn't)
| export class CcBreadcrumbs extends LitElement { | ||
| static get properties() { | ||
| return { | ||
| disabled: { type: Boolean }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: this property is not implemented
| 'cc-cellar-explorer.error': `Une erreur est survenue pendant le chargement`, | ||
| //#endregion | ||
| //#region cc-cellar-object-list | ||
| 'cc-cellar-object-list.back-to-bucket-list': `Retour à la list des buckets`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix:
| 'cc-cellar-object-list.back-to-bucket-list': `Retour à la list des buckets`, | |
| 'cc-cellar-object-list.back-to-bucket-list': `Retour à la liste des buckets`, |
| white-space: wrap; | ||
| word-wrap: anywhere; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick(not sure): Claude tells me this, I'm not sure, I'll go and dig when I have time because I don't know enough about these 🙈
white-space: wrap; // Invalid - should be 'normal'
word-wrap: anywhere; // This is also deprecated, prefer overflow-wrap: anywhere
| try { | ||
| const signedUrl = await this.#cellarClient.getObjectSignedUrl(this.#bucketName, objectKey); | ||
|
|
||
| const element = document.createElement('a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: if we really want to go down that road, we should use the download attribute on the link so that it has the proper behavior.
Still I think it's a smell that we might be doing this wrong: if what we need is the link behavior, we should use a link from the start because it will also bring the relevant semantics.
| /** @type {Ref<CcBreadcrumbs>} */ | ||
| this._breadcrumbsRef = createRef(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: the breadcrumbsRef is not initialized on the element
Summary
cc-cellar-object-listcomponent to browse and manage objects (files/directories) within a Cellar bucketcc-breadcrumbscomponent for hierarchical path navigationenableCopyToClipboardoption to cc-grid link cellsAbortableutility class to a generic lib for request cancellationcc-cellar-explorerDetails
New components
cc-cellar-object-listA full-featured component for browsing Cellar bucket contents with:
cc-breadcrumbsA reusable breadcrumb navigation component that:
Enhancements
cc-grid: Link cells can now display a copy-to-clipboard button viaenableCopyToClipboard: truecc-cellar-explorer: Uses Abortable for smart request cancellation when navigating between buckets/pathsRefactoring
How to review
cc-breadcrumbsstories render correctly and click events workcc-gridstoriescc-cellar-object-liststories