Skip to content

Tooltips for CSV import and import page refactoring - #2655

Merged
dae merged 15 commits into
ankitects:mainfrom
RumovZ:import-csv-tooltips
Sep 13, 2023
Merged

dae merged 15 commits into
ankitects:mainfrom
RumovZ:import-csv-tooltips

Conversation

@RumovZ

@RumovZ RumovZ commented Sep 12, 2023

Copy link
Copy Markdown
Collaborator

As always, please go ahead if you see any room for improvement, especially with the fluent strings or the styling, which I'm not super confident with.
@abdnh, your input is welcome as well, of course!

@dae dae left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will try to review this properly tomorrow, but one issue I noticed so far: when importing an .apkg file, progress is no longer shown correctly.

Comment thread ftl/core/importing.ftl Resolved
Comment thread ftl/core/importing.ftl Resolved
Comment thread ftl/core/importing.ftl Resolved
Comment thread ftl/core/importing.ftl Resolved
RumovZ and others added 3 commits September 13, 2023 17:03
@RumovZ

RumovZ commented Sep 13, 2023

Copy link
Copy Markdown
Collaborator Author

Thanks. Should've tested with larger imports.

e.g. 500: Failed to read '/home/dae/foo2.csv':<br>stream did not contain valid UTF-8

I thought about using {@html ...} here, but that's a potential security issue,
as the filename is not something we control.

@dae dae left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks Rumo! This is a nice improvement. And no problem, that's what review is for :-)

export let value = 0;
type T = $$Generic;

export let value: T;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice

<Row><FileOptions {state} /></Row>
<Row><ImportOptions {state} /></Row>
<Row><FieldMapper {state} /></Row>
</ImportPage>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice and clean!

Comment thread ts/import-csv/index.ts
notetypeColumn: tryGetNotetypeColumn(metadata),
deckId: tryGetDeckId(metadata),
deckColumn: tryGetDeckColumn(metadata),
state: new ImportCsvState(path, notetypes, decks, metadata),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, seems like a nice change

@dae
dae merged commit 850043b into ankitects:main Sep 13, 2023
@RumovZ
RumovZ deleted the import-csv-tooltips branch September 14, 2023 08:40
@RumovZ RumovZ mentioned this pull request Sep 14, 2023
12 of 27 tasks
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.

2 participants