application-research / estuary-www

https://estuary.tech
https://estuary.tech
Other
35 stars 31 forks source link

Remove files col + Show size and created at in files table #117

Closed neelvirdy closed 1 year ago

neelvirdy commented 1 year ago
  1. https://github.com/application-research/estuary/pull/631 removed aggregate rows from /content/stats, so the Files column becomes irrelevant and is removed in this PR
  2. Size and createdAt were added to the /content/stats response data, and is rendered in this PR
  3. some col width tuning

To be released after estuary 0.2.1.

before: image

after: image

en0ma commented 1 year ago

@neelvirdy can we add the status of the file as well, so users have visibility into what state the content is in

neelvirdy commented 1 year ago

@neelvirdy can we add the status of the file as well, so users have visibility into what state the content is in

@en0ma i looked into this for a bit and i think it's worth tracking as a separate PR. it feels sorta odd since most of the time you will just see "pinned" for every row. would want to do a more live-status tracking after consolidating the Add page with the FilesTable page so it's more of a single live-view and you can render the in-progress row differently from the pinned rows, but not literally have a column that says "pinned" vs "pinning" vs "failed". lmk if you disagree or if you meant something other than just pinning status!

en0ma commented 1 year ago

@neelvirdy can we add the status of the file as well, so users have visibility into what state the content is in

@en0ma i looked into this for a bit and i think it's worth tracking as a separate PR. it feels sorta odd since most of the time you will just see "pinned" for every row. would want to do a more live-status tracking after consolidating the Add page with the FilesTable page so it's more of a single live-view and you can render the in-progress row differently from the pinned rows, but not literally have a column that says "pinned" vs "pinning" vs "failed". lmk if you disagree or if you meant something other than just pinning status!

It's ok to do it in a follow-up PR. Not sure what you mean by live status tracking, but what I was asking is that we show the pin state and most users would want that - the pinning API gives them this but the UI does not. Every file in estuary is either in the pinning, pinned or failed state

kelindi commented 1 year ago

@neelvirdy have you checked the responsiveness for the table after adding the additional columns? Make sure it looks good on all the screen sizes including mobile. Also did we want to get rid of the Files column?

neelvirdy commented 1 year ago

@neelvirdy have you checked the responsiveness for the table after adding the additional columns? Make sure it looks good on all the screen sizes including mobile. Also did we want to get rid of the Files column?

yep i did a quick check shrinking my window to a variety of sizes, looks sane. and yep the files column is always 1 for non-aggregates and part of the estuary PR linked is to stop including aggregates in the /content/stats query, since they're not user-created they shouldn't care or necessarily even know they exist

jimmylee commented 1 year ago

Sorry I didn't fully track the discussion and thought there was still more to work on, @neelvirdy you're going to have to fix conflicts.

neelvirdy commented 1 year ago

@neelvirdy can we add the status of the file as well, so users have visibility into what state the content is in

@en0ma i looked into this for a bit and i think it's worth tracking as a separate PR. it feels sorta odd since most of the time you will just see "pinned" for every row. would want to do a more live-status tracking after consolidating the Add page with the FilesTable page so it's more of a single live-view and you can render the in-progress row differently from the pinned rows, but not literally have a column that says "pinned" vs "pinning" vs "failed". lmk if you disagree or if you meant something other than just pinning status!

It's ok to do it in a follow-up PR. Not sure what you mean by live status tracking, but what I was asking is that we show the pin state and most users would want that - the pinning API gives them this but the UI does not. Every file in estuary is either in the pinning, pinned or failed state

yup understood - i meant that i built it out and it felt off because every row that shows up is "pinned". something about the request flow currently prevents "pinning" states from appearing as rows - or maybe there was a filter somewhere i didn't find. so in the end every row just had a column with the word "pinned" in it and it felt off. i think there is a more intuitive way of communicating pin state other than the extra column so would rather track it separately

en0ma commented 1 year ago

@neelvirdy can we add the status of the file as well, so users have visibility into what state the content is in

@en0ma i looked into this for a bit and i think it's worth tracking as a separate PR. it feels sorta odd since most of the time you will just see "pinned" for every row. would want to do a more live-status tracking after consolidating the Add page with the FilesTable page so it's more of a single live-view and you can render the in-progress row differently from the pinned rows, but not literally have a column that says "pinned" vs "pinning" vs "failed". lmk if you disagree or if you meant something other than just pinning status!

It's ok to do it in a follow-up PR. Not sure what you mean by live status tracking, but what I was asking is that we show the pin state and most users would want that - the pinning API gives them this but the UI does not. Every file in estuary is either in the pinning, pinned or failed state

yup understood - i meant that i built it out and it felt off because every row that shows up is "pinned". something about the request flow currently prevents "pinning" states from appearing as rows - or maybe there was a filter somewhere i didn't find. so in the end every row just had a column with the word "pinned" in it and it felt off. i think there is a more intuitive way of communicating pin state other than the extra column so would rather track it separately

well, if all your files are pinned, it will show pinned. Try testing for failed and pinning states, you will see the state show up

neelvirdy commented 1 year ago

@neelvirdy can we add the status of the file as well, so users have visibility into what state the content is in

@en0ma i looked into this for a bit and i think it's worth tracking as a separate PR. it feels sorta odd since most of the time you will just see "pinned" for every row. would want to do a more live-status tracking after consolidating the Add page with the FilesTable page so it's more of a single live-view and you can render the in-progress row differently from the pinned rows, but not literally have a column that says "pinned" vs "pinning" vs "failed". lmk if you disagree or if you meant something other than just pinning status!

It's ok to do it in a follow-up PR. Not sure what you mean by live status tracking, but what I was asking is that we show the pin state and most users would want that - the pinning API gives them this but the UI does not. Every file in estuary is either in the pinning, pinned or failed state

yup understood - i meant that i built it out and it felt off because every row that shows up is "pinned". something about the request flow currently prevents "pinning" states from appearing as rows - or maybe there was a filter somewhere i didn't find. so in the end every row just had a column with the word "pinned" in it and it felt off. i think there is a more intuitive way of communicating pin state other than the extra column so would rather track it separately

well, if all your files are pinned, it will show pinned. Try testing for failed and pinning states, you will see the state show up

i tested with a large file so it should've been pinning for more than a few seconds and confirmed it didn't appear in the table at all until it reached pinned state. again i didn't fully dig into why, but it seemed not worth the time to not bother right now. didn't know how to force a failure so didn't really bother trying to test a failed state.

regardless - agreed it can be a different PR, although i'm not planning to track it for now

kelindi commented 1 year ago

@neelvirdy The styling on mobile still needs some fixing. Attached a screenshot from an Iphone12. Let me know if you need help fixing it on mobile. Also is there a reason we aren’t using a date format such as 12-06-2022 instead of the long form written date? image

neelvirdy commented 1 year ago

@kelindi ack - did some more tuning

image

neelvirdy commented 1 year ago

@kelindi thanks for showing me the device selector - here's the new styles:

iPhone SE: image

iPad Air: image

Desktop: image

kelindi commented 1 year ago

Styling LGTM