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

Issue 16511, add time for when user last logged in, in the grid user card #2205

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

EmanuelGustafzon
Copy link
Contributor

@EmanuelGustafzon EmanuelGustafzon commented Aug 10, 2024

Description

I used the Date object and added user.lastLoggedinDate to get the local date and time, then i formatted the time and added it to the DOM.

Link to the issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

It makes you being able to see what time users was last logged in.

How to test?

I did not write test for this one, if you want me to please provide me some guidance

Checklist

  • If my change requires a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

Copy link

sonarcloud bot commented Aug 10, 2024

@Migaroez
Copy link
Contributor

Hey @EmanuelGustafzon, thank you for the PR.
I have changed some formatting in your submission (remove spaces in markdown checkbox notation) and added a link to the issue.

I will check with the front-end team if they can have a look at your changes.

@EmanuelGustafzon
Copy link
Contributor Author

Thank you @Migaroez for your support, I appreciate it and just let me know if I need to change something.

@Migaroez
Copy link
Contributor

Hey @EmanuelGustafzon, I had a chat with @nielslyngsoe and he approves of the code as a temporary fix for the missing time. Ideally he would like to have an Umbraco-UI library component that takes a date-time and displays it in a way as described in the story linked in the issue (umbraco/Umbraco.UI#143)

We do understand if this would be to much to undertake for you and will approve this PR to fix the issue as is if you are indeed not up for the more comprehensive solution.

@EmanuelGustafzon
Copy link
Contributor Author

Great to hear the code is approved, @Migaroez! That’s an encouragemnet. I'm definitely up for the challenge of adding a UI element.

My first impression of Umbraco and the community has been fantastic, and my goal is to contribute regularly. I’ve been looking for an open-source project to get involved in, and this feels like the right fit.

Should I work within the Umbraco UI Library repository for this?

@Migaroez
Copy link
Contributor

Wonderful to hear that you are enjoying your time with us and the community 🥳

Indeed, you would make a PR against the UI library and when that is merged in the code you made here together with the date display that is already there would be replaced by the component.

New components added to the UI library need to have an addition to the Storybook as well with a nice description and all the different uses cases the component supports. Don't worry about this to much yet, but it will be required before the PR is accepted. The team will help guide you after the initial creation of the component.

I have not developed with Storybook myself, but from talking to @nielslyngsoe I have gathered that if you run the UI library project and thus Storybook in dev mode, it kind of acts like a playground (think JSFiddle and the like) where you can develop and document the component at the same time. Hope this barebones explanation pushes you in the right direction, a more comprehensive contribution guideline for the UI library is on the todo list 🙈

Thanks again for the interest in the project(s) and the work you have already put in #h5yr!

@EmanuelGustafzon
Copy link
Contributor Author

Alright cool! I think I got it right I just set up the project with storybook and added a new initial component and it seems quite straightforward so far.

I guess the component should work in different contexts? I mean not only "user was logged in 1 hour ago" but also let's say, "the page was updated 1 hour ago" etc.

@Migaroez
Copy link
Contributor

From what I understand from the description, the component should not even be aware of the context. The only thing it should take is a datetime and render a relative short time phrase for the difference in time.

The component can then be used like so (forgive my syntax mistakes)

<userLastLoginComponent>
<p>Last Logged in</p>
<p><uui-time-ago data=$user.lastLoginDateTime></uui-time-ago></p>
</userLastLoginComponent>

Which would result in

<p>Last Logged in</p>
<p>2 hours ago</p>

Or

<LastPublishedTime>
<p>This page was last published <uui-time-ago data=$page.publishDate></uui-time-ago>.</p>
</LastPublishedTime>

Which would result in

<p>This page was last published yesterday.</p>

I had not realized that @bjarnef already has a draft PR for this functionality 🙈, maybe the 2 of you can collaborate?

@EmanuelGustafzon
Copy link
Contributor Author

Yes that's what I thought great, I was looking at the draft PR I think the code should be simplified.

I see that he also added in the future which is a good point.

So should I start building a new component and add a PR or how should I proceed?

@EmanuelGustafzon
Copy link
Contributor Author

Bjarnaf is already in process with this issue he said and waiting for some feedback so I let it to him.

If you have any other job in mind I am happy to contribute!

Copy link
Member

@nielslyngsoe nielslyngsoe left a comment

Choose a reason for hiding this comment

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

Thanks @EmanuelGustafzon — this works well, so I'm happy to merge.
As the conversation above states then it would be nice to improve the experience showing a description of the time relative to now. But that is an improvement outside the scope of this PR.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants