rauno56 / climbing-card-check

climbing-card-check.vercel.app
0 stars 2 forks source link

feat: load semantic headers from the second row #27

Closed rauno56 closed 7 months ago

vercel[bot] commented 7 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
climbing-card-check ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 3, 2024 3:04pm
taavilooke commented 7 months ago

It seems to work correctly, but I think the row headers in the database should still be in Estonian. The point is that the first row is what ever is in the form and the second row the human readable headers. Since everything else is in Estonian, why should the headers be in English?

rauno56 commented 7 months ago

... I think the row headers in the database should still be in Estonian

Aren't we going to hide the "column keys"(to distinguish them from human-readable "column headers")?

In that case, I'd say it's solely a implementation detail and "belongs" to the API, not the registry. The code is in English -> keys are in English to match the implementation.

They are just a correlation ID. I'd strongly encourage us to use the variable names in the code there, but they can technically be anything. Text with spaces and mixed casing is just more error-prone.

taavilooke commented 7 months ago

My plan was to hide the first row and show the second row in the database, because the first row will always change according to how the questions are labeled in the forms. I guess we could also hide the second row and show the first row for human readable headers. There are a few labels that don't make so much sense in Estonian, but I guess we can add notes to the fields to explain the meaning. It's better than adding a third row for the labels and hiding the first two rows,

On Tue, Jan 30, 2024 at 3:05 PM Rauno Viskus @.***> wrote:

... I think the row headers in the database should still be in Estonian

Aren't we going to hide the "column keys"(to distinguish them from human-readable "column headers")?

In that case, I'd say it's solely a implementation detail and "belongs" to the API, not the registry. The code is in English -> keys are in English to match the implementation.

They are just a correlation ID. I'd strongly encourage us to use the variable names in the code there, but they can technically be anything. Text with spaces and mixed casing is just more error-prone.

— Reply to this email directly, view it on GitHub https://github.com/rauno56/climbing-card-check/pull/27#issuecomment-1916804881, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG2OPM2PJAV2O6MUTPKQNLTYRDV3JAVCNFSM6AAAAABCODBIHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJWHAYDIOBYGE . You are receiving this because your review was requested.Message ID: @.***>

taavilooke commented 7 months ago

Hmm, I think it works as expected then. :)

On Tue, Jan 30, 2024 at 3:14 PM Rauno Viskus @.***> wrote:

@.**** commented on this pull request.

In api/_db.data-utils.js https://github.com/rauno56/climbing-card-check/pull/27#discussion_r1471186015 :

  • });
  • // no result
  • assert(filteredRows.length, 'Not found');
  • // more than one result, assume an filtering error
  • if (filteredRows.length > 1) {
  • console.log('Filtered more than one row', filteredRows);
  • throw new Error('More than one row');
  • }
  • const row = filteredRows[0];
  • const certificate = normalizeCertificate(row[certificateColumnIdx] ?? '');
  • const formFillDate = parseDate(row[formFillTimeColumnIdx]);
  • const expiryDate = parseDate(row[expiryDateColumnIdx]) || getExpiryTimeFromFormFillTime(formFillDate) || null;

Then null is returned here and the UI shows it as expired.

What should happen?

— Reply to this email directly, view it on GitHub https://github.com/rauno56/climbing-card-check/pull/27#discussion_r1471186015, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG2OPM3745WX45YDHX5BTYDYRDWZRAVCNFSM6AAAAABCODBIHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNJRGIZTINBVGU . You are receiving this because your review was requested.Message ID: @.***>

rauno56 commented 7 months ago

Aah... I see where you came from, now, advocating for the Estonian.

Yeah, let's definitely hide the second row and use machine-readable keys there.

  1. The first one is in Estonian anyway,
  2. The visible row should be something that correlates to the form or what humans talk about, thus better to be in sync with what's on form etc.
rauno56 commented 7 months ago

@taavilooke, also let me know once the headers are changed back to match the code. We cannot merge this before.