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

Team pick icon + ratings #25

Merged
merged 16 commits into from
Sep 16, 2023
Merged

Team pick icon + ratings #25

merged 16 commits into from
Sep 16, 2023

Conversation

speedyman08
Copy link
Contributor

image

package.json Outdated Show resolved Hide resolved
src/app/pages/level/level.component.html Outdated Show resolved Hide resolved
src/app/pages/level/level.component.html Outdated Show resolved Hide resolved
@speedyman08 speedyman08 changed the title Team pick icon Team pick icon + ratings Sep 11, 2023
@speedyman08
Copy link
Contributor Author

Screenshot_20230912_002017
This is how the new tooltip looks like. Thoughts?

@speedyman08 speedyman08 requested a review from jvyden September 11, 2023 18:27
Copy link
Member

@jvyden jvyden left a comment

Choose a reason for hiding this comment

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

I think it looks good, but I don't like the implications of the CSS behind it.

src/styles.css Outdated Show resolved Hide resolved
@speedyman08
Copy link
Contributor Author

speedyman08 commented Sep 12, 2023

I've gotten it working under tailwind. Just a few issues though:

  1. the font is incorrect so i have to use font-family to correct it to Rubik
  2. for some reason every space that is in the span acts as a newline, and i'm not sure on why that is happening. i didn't mess around with white-space fixed after adding white-space: nowrap;

@speedyman08
Copy link
Contributor Author

2023-09-12.19-33-26.mp4

@speedyman08 speedyman08 requested a review from jvyden September 12, 2023 16:36
@speedyman08
Copy link
Contributor Author

speedyman08 commented Sep 12, 2023

The tooltip can now be used anywhere now. To use it, add a 'tooltip-container' class to an element, and add the 'tooltip' component as a child to it. Also, the width is not hardcoded anymore.

Copy link
Member

@jvyden jvyden left a comment

Choose a reason for hiding this comment

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

A lot better, just nitpicks at this point

src/app/pages/level/level.component.html Outdated Show resolved Hide resolved
src/styles.css Outdated Show resolved Hide resolved
@jvyden
Copy link
Member

jvyden commented Sep 12, 2023

The font not automatically applying is definitely unintentional behavior so I'll look at it when I get a chance. Ideally there should be 0 CSS rules like the rest of refresh-web.

@speedyman08 speedyman08 requested a review from jvyden September 14, 2023 11:58
@jvyden jvyden removed their request for review September 16, 2023 00:44
@jvyden
Copy link
Member

jvyden commented Sep 16, 2023

Removing the request for review since my previous review pass still holds for now.

As a side-note, apologies for the delay. Haven't been feeling well the past couple of days but things have been steadily improving, and this PR should hopefully speed up now 😁

Copy link
Member

@jvyden jvyden left a comment

Choose a reason for hiding this comment

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

I made some changes so it's easier to use from an HTML standpoint, and I'm now pretty happy with how this turned out. I think there's some design changes to be made (e.g. shadowing & better contrast in general) but this can be addressed later after I've played around in Figma enough.

Apologies for the long review process and the dramatic changes necessary it took to get here, but regardless, thank you for the contribution!

@jvyden jvyden merged commit 9a7e53e into LittleBigRefresh:master Sep 16, 2023
1 check passed
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.

2 participants