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

Avoid duplicate user-local-time #6037

Merged
merged 5 commits into from
Oct 3, 2022
Merged

Avoid duplicate user-local-time #6037

merged 5 commits into from
Oct 3, 2022

Conversation

fregante
Copy link
Member

@fregante fregante commented Oct 3, 2022

Test URLs

Screenshot

Screen Shot

@fregante fregante marked this pull request as ready for review October 3, 2022 07:10
Copy link
Member Author

@fregante fregante left a comment

Choose a reason for hiding this comment

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

The feature has a lot of awkward logic due to the hovercard behavior. It can probably be avoided with some tactical CSS that adds the required padding automatically to all user hovercards so that the initial calculation by GitHub takes it into account. Example:

.Popover-message:has([aria-label="user avatar"]):not(:has(.octicon)) {
  padding-bottom: 26px;
}

Plus now better selectors are available thanks to the aria-label attributes. The feature would look like:

observe('user Details Container without time', container => {
  if (cached time is `false`) {
    return
  }
  const user = container.user ?? globalUser
  container.append(<DynamicUserTime {user}>)
})

);

void display({datePromise, placeholder, container});
return container;
Copy link
Member Author

Choose a reason for hiding this comment

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

@kidonng I reverted #5779 because the temporary duplicate code ended up being permanent and the feature was unmaintainable as is.

@fregante fregante merged commit 785e725 into main Oct 3, 2022
@fregante fregante deleted the ajax-fix branch October 3, 2022 07:18
@fregante fregante added the bug label Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Duplicate on user-local-time (official feature now)
1 participant