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

fix: schedule hovering style #157

Merged
merged 4 commits into from
Sep 28, 2021

Conversation

yuyanghh
Copy link
Contributor

Types of changes

  • Bugfix

Description

Follow Figma design to fix the style of schedule table on mouse hovering

2021-08-30.9.08.36.mov

@@ -137,7 +137,8 @@ export default {
},
methods: {
getKeynoteId(keynote) {
return keynote.speaker.name_en_us.split(' ').join('_')
// adjust for schedule data format
return keynote.speaker.name_zh_hant.split(' ').join('_')
Copy link
Member

Choose a reason for hiding this comment

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

why does this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that the scroll behavior on production page is failed because the anchor and the url are inconsistent.
Reproduce steps:

  1. go to PyCon Taiwan 2021 official site
  2. head to schedule page
  3. click on the first keynote by 魏澤人
  4. being directed to keynote page without scroll behavior

Thus, modifying the data used as id can fix the issue.

Copy link
Member

Choose a reason for hiding this comment

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

I always get the english content on schedule page (cannot change with locale switch) and thus the generated id is correct for me. I assume you get the zh-hant content on this page?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes ~ I thought that's why the scroll behavior failed.

截圖 2021-09-26 下午12 05 10

Comment on lines 71 to 92
@media (hover: hover) {
&:hover {
color: #000000;
background-color: #d1ac23;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

just curious. why can't we get rid of media query here?

Suggested change
@media (hover: hover) {
&:hover {
color: #000000;
background-color: #d1ac23;
}
}
&:hover {
color: #000000;
background-color: #d1ac23;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without hover media query, mobile users could unintentionally active hover state.

@flynnhou
Copy link
Collaborator

flynnhou commented Sep 8, 2021

@yuyanghh Thanks for submitting the PR!

A few things to point out here:

  1. The hover effect should only apply to the ones having links to navigate to. Blocks like room, break, etc. shouldn't change the background color for they're meant to be static.
  2. I'm curious about if the media query could be replaced by pointer events, e.g., pointerenter? Utilize such events to toggle the classes, and then define the hover style in css selectors with the classes so that we don't need to specify the media query. It looks a bit ad hoc and wet to me. But if pointer events don't work as you wanted, then let's leave it like this.

Copy link
Member

@mattwang44 mattwang44 left a comment

Choose a reason for hiding this comment

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

LGTM but I'll revert the fix for the scroll bar issue before merging this PR.
The root cause of scroll bar issue is that we did not apply i18n for this page (see #174), and I'll take over this part!

@mattwang44 mattwang44 merged commit ad98eb5 into pycontw:main Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants