astralapp / astral

Organize Your GitHub Stars With Ease
https://astralapp.com
BSD 3-Clause "New" or "Revised" License
3.18k stars 141 forks source link

feat: Add a relative updatedAt timestamp on dashboard #332

Closed tomershvueli closed 3 years ago

tomershvueli commented 3 years ago

Bring in the updatedAt attribute for each repo and display it in the dashboard list in a relative format. All tests are passing locally.

Eventually it'd be great to add this to Predicates/smart filtering, but that's a bit more of a process since it'll probably need a date picker component, etc.

Screenshot: Screen Shot 2021-01-10 at 1 39 04 PM I purposefully put a screenshot up with an overflow, not sure how we want to handle that, but I think it doesn't look horrible now, especially since it autocorrects itself a bit and doesn't overlap with other objects on the page.

schmurfy commented 3 years ago

looks nice :) I realize that this is not the same complexity but would it be possible to also have it as a filter ?😁

tomershvueli commented 3 years ago

looks nice :) I realize that this is not the same complexity but would it be possible to also have it as a filter ?😁

I can try and take a quick stab, but as we both mentioned, the scope is larger than just displaying the updatedAt value.

syropian commented 3 years ago

@tomershvueli Thanks for the PR, a few things:

  1. I'm not a fan of how it wraps when things get too long - the alignment is weird and it just looks kinda broken. Seems like a bit of an edgecase (mostly caused by a long "latest release" version), but it'd be nice if it looked good in all cases. I might have to ponder this one myself a bit for a good design solution (perhaps sticking it on the top right?)
  2. What's up with the composer lockfile changes? Doesn't look like anything changed in the actual composer file, but the new lock changes are causing the CI tests to fail
  3. It probably make sense to just pull in a date library to do the relative format instead of writing (and thus maintaining) a custom function. If we eventually want to use the updatedAt for sorting/filtering we'll probably need one anyway. I'm thinking either dayjs or date-fns.
tomershvueli commented 3 years ago

@syropian thanks for the response.

  1. I agree it's not ideal. I was actually thinking about putting it in the top right of the Star component, but realized it would conflict if a repo was archived: image I don't want to make the component too busy since it looks quite nice as it is now. Open to thoughts on how you want to proceed. Unless if a repo is archived we don't show updatedAt since it doesn't really matter? 1a. While here, I was wondering if we wanted to put a standard formatting for Last Updated on the details/Readme view, like so: image Thoughts? Preference on 'standard' formatting?
  2. Not sure what happened here - any thoughts? To test locally, do I just do phpunit.phar ./? I haven't really worked with composer or phpunit much so any input here is welcome.
  3. Sure! I was trying to keep it light and chose to not bring in a 3rd party library on purpose, but happy to. I'll take a look between those 2 modules and bring one in!
syropian commented 3 years ago

@tomershvueli Oh good point on the archived bit, I forgot about that. One idea I had was condensing the relative time format (and perhaps adding title/aria attributes that contain the full date). I've seen some apps that just format relative like 20s, 3m, 6h, 14d, 7mo, 1y. That might be short enough to avoid the wrapping in 99% of cases.

I do like the idea of also showing the date in the info pane - the styles might have to be tweaked slightly.

For tests, running vendor/bin/phpunit should suffice.

tomershvueli commented 3 years ago

@syropian Take a look at the latest commit. The short hand notation for updatedAt is in place, as well as in the Readme view. image

Unfortunately I'm not sure what's happening with the tests. When I run locally, there's only 1 test that's failing and it's it_can_pass_a_cursor_to_fetch_a_certain_page_of_stars, which doesn't seem to have to do with the composer file. Can you pull down this branch and try it on your machine?

tomershvueli commented 3 years ago

Another thing to note: I realized that the updated date sometime didn't align with what I saw on the repo - this is due to the fact that updatedAt is officially anytime anything in the repo has been updated, whereas we could instead bring in pushedAt for the latest commit to the repo (though this might also not necessarily be in the main branch). What do you think is most appropriate for this value?

syropian commented 3 years ago

this is due to the fact that updatedAt is officially anytime anything in the repo has been updated

Good question, personally the last pushed at date (regardless of branch) makes sense to me.

tomershvueli commented 3 years ago

@syropian agreed! Just pushed up a quick fix for that.

Let me know your thoughts on the tests. I think besides that this PR should be good to merge. I'll work on adding this value to the predicate/filter in a separate branch and PR.

syropian commented 3 years ago

@tomershvueli Thanks! I'll take a peak at the tests issue after I'm done work 👍🏻

syropian commented 3 years ago

@tomershvueli By chance did you run composer update instead of composer install when installing the PHP dependencies? Either way, since you didn't make any changes to any composer packages you are probably safe to just back out your changes to composer.lock to get the tests to pass in CI.

tomershvueli commented 3 years ago

@syropian it's very possible I ran composer update when I should've run something else. But take a look at the latest commit. I reverted composer.lock to its original contents from before the PR and the tests are still failing :/ thoughts?

syropian commented 3 years ago

@tomershvueli Oh, this is an issue on the master branch, I didn't even notice! I'm ok to merge this, I'll fix the composer issue myself 👍🏻

syropian commented 3 years ago

Master is now fixed as well! The build failures were twofold:

  1. GitHub Actions use PHP 8 by default, so I had to specify I wanted 7.4
  2. Astral's version of laravel/framework was not compatible with Composer v2.
tomershvueli commented 3 years ago

@syropian awesome! Thanks for the help with the final push - glad it worked. I hope to have another PR soon with Updated At as a value in the Smart Filter, just trying to familiarize myself with Vue a bit :)