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

Introduce a Llaminator class, be able to list multiple items #73

Merged
merged 1 commit into from
Jan 11, 2022

Conversation

beverloo
Copy link
Collaborator

Fixes #72

Only functional change is in Llaminator.populate().

clearContainer() {
const { container } = this.elements;

while (container.firstChild)
Copy link
Collaborator

Choose a reason for hiding this comment

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

!$ npx eslint src/llaminator.ts

/home/glenn/llaminator/src/llaminator.ts
  46:14  warning  Missing JSDoc comment               require-jsdoc
  62:7   error    Expected { after 'while' condition  curly

✖ 2 problems (1 error, 1 warning)
  1 error and 0 warnings potentially fixable with the `--fix` option.

We can ignore the jsdoc warning. It might take some time (and separate PRs) to fix all the lint issues with existing files, but we might as well make sure new ones are compliant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

I can't actually run the linter locally yet without a ton of noise, because all the line endings are CRLF for me. Yay Windows :) Will figure that out separately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you probably want to change your git autocrlf settings for the project to input (or false, but preferable input), then possibly re-checkout. As long as your editor can handle that, I think it should solve the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also I just learned about the editorconfig format example - might be a convenient way to standardize our editors to use the same style as our linters

* Is given a series of HTML elements (`LlaminatorElements`) in which the app is to be rendered.
*/
export class Llaminator {
private elements: LlaminatorElements;
Copy link
Collaborator

Choose a reason for hiding this comment

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

readonly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

*/
export class Llaminator {
private elements: LlaminatorElements;
private storage: Promise<LlamaStorage>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably also readonly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


// Set up event listeners on the elements.
this.elements.select.addEventListener(
'fileselected', Llaminator.prototype.onFileSelected.bind(this));
Copy link
Collaborator

Choose a reason for hiding this comment

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

this.onFileSelected doesn't work? :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, then the this value in the onFileSelected function would be bound to the input element.

* Called when a file has been selected in the <llama-select-fab> that is to be stored.
*
* @param {Event} event The event, which is a `CustomEvent` with its detail being a File instance.
* @todo Can we provide stronger typing for our own custom events?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's pretty close to what I found. Still, we should try, I don't think that it's impossible.

llama-item {
margin-bottom: 2rem;

/** TODO: Use a stack based on flexbox */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've taken brief looks at flexbox and grid layout before. I think I found grid to be much more manageable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm fairly sure I disagree for a simple stack like this, which shouldn't be more complicated than...

main {
  display: flex;
  flex-direction: column;
  row-gap: 2rem;
}

However, it's a TODO because we should try properly :)

https://css-tricks.com/snippets/css/a-guide-to-flexbox/

@beverloo beverloo merged commit 365bbd1 into GoogleChromeLabs:main Jan 11, 2022
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.

llama-item doesn't update properly
2 participants