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

feat(website): home page, user page and player #116

Merged

Conversation

lennartkloock
Copy link
Member

@lennartkloock lennartkloock commented Aug 5, 2023

Proposed changes

Frontend

  • New home page design
  • New user page design
    • Player
    • Chat

Backend

  • Adds display_color to user
    • will be randomly generated when registering
  • activeStreamsByUserId GQL endpoint to fetch all active streams by user id (this may become obsolete with the new data structure soon)

Types of changes

What types of changes does your code introduce to Scuffle?
Put an x in the boxes that apply

  • Bugfix (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 not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

@lennartkloock lennartkloock changed the title feat(website): user page feat(website): user page and player Aug 5, 2023
@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Merging #116 (fa5418c) into feature/website (ea2387a) will decrease coverage by 0.15%.
Report is 22 commits behind head on feature/website.
The diff coverage is 40.00%.

@@                 Coverage Diff                 @@
##           feature/website     #116      +/-   ##
===================================================
- Coverage            73.97%   73.83%   -0.15%     
===================================================
  Files                  220      221       +1     
  Lines                17538    17608      +70     
===================================================
+ Hits                 12973    13000      +27     
- Misses                4565     4608      +43     
Files Changed Coverage Δ
backend/api/src/api/v1/gql/mod.rs 81.42% <0.00%> (-18.58%) ⬇️
backend/api/src/api/v1/gql/models/stream.rs 6.66% <6.66%> (ø)
backend/api/src/dataloader/stream.rs 52.94% <17.64%> (-35.30%) ⬇️
backend/api/src/database/user.rs 89.84% <95.45%> (+1.16%) ⬆️
backend/api/src/api/v1/gql/models/user.rs 75.65% <100.00%> (+0.43%) ⬆️
backend/api/src/global/mod.rs 69.91% <100.00%> (+0.24%) ⬆️

... and 1 file with indirect coverage changes

@lennartkloock lennartkloock changed the title feat(website): user page and player feat(website): home page, user page and player Aug 20, 2023
@lennartkloock lennartkloock marked this pull request as ready for review August 22, 2023 17:58
@lennartkloock lennartkloock requested review from a team as code owners August 22, 2023 17:58
%sveltekit.head%
</head>
<body data-sveltekit-preload-data="hover">
<div style="display: contents">%sveltekit.body%</div>
%sveltekit.body%
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct? I believe svelte recommends to have a div wrap around the generated code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it was in the template like this but I couldn't find any reason for the div. Do you know why it's there?

Comment on lines 27 to +29

<style lang="scss">
@import "../assets/styles/variables.scss";
@import "../../assets/styles/variables.scss";
Copy link
Member

Choose a reason for hiding this comment

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

Can we use $ path syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find any documentation for that syntax. I know it works in svelte script tags but I don't think it works in scss style tags.

@@ -0,0 +1,8 @@
<script lang="ts">
export let size: number = 24;
Copy link
Member

Choose a reason for hiding this comment

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

previously i did this with rem and fontsize, curious what your thoughts are about that approach? Is this better todo?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is, as far as I know, I can't apply any styles to the icon from outside of the icon component itself. Which means I would need a wrapper element to apply the styles to which seems unnecessary.

Copy link
Member

@TroyKomodo TroyKomodo left a comment

Choose a reason for hiding this comment

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

Lgtm just fix up those few things

@TroyKomodo
Copy link
Member

We can ignore the audit failure the CVE was dropped today, also LAPIN (the library which is affected, isnt used in my branch anymore since we stopped using RMQ)

@lennartkloock lennartkloock force-pushed the lennart/frontend-user-page branch 2 times, most recently from dcb9c4d to b1041e3 Compare August 23, 2023 15:22
- New home page design
- New user page design
  - Player
  - Chat

- Adds `display_color` to user
  - will be randomly generated when registering
- `activeStreamsByUserId` GQL endpoint to fetch all active streams
  by user id (this may become obsolete with the new data structure soon)
@TroyKomodo
Copy link
Member

4x

@TroyKomodo TroyKomodo merged commit fa5418c into ScuffleTV:feature/website Aug 23, 2023
3 of 5 checks passed
@lennartkloock lennartkloock deleted the lennart/frontend-user-page branch November 14, 2023 13:02
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