Skip to content

Conversation

@Cykotech
Copy link
Contributor

@Cykotech Cykotech commented Jan 6, 2025

What issue is this solving?

Closes #28

Description

Merged both design concepts.

Any helpful knowledge/context for the reviewer?

  • Any new dependencies to install?
  • Any special requirements to test?
  • Any UI changes? Include screenshots if so.

image
image

Feelings gif (optional)

alt text

Please make sure you've attempted to meet the following coding standards

  • Code has been tested and does not produce errors
  • Code is readable and formatted
  • There isn't any unnecessary commented-out code

Copy link
Collaborator

@aedwardg aedwardg 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 this is a good starting place, though ideally I'd like to keep PRs much smaller than this. This may just be a side-effect of not having a designer on the team though.

I think this is good to merge except there seems to be some git weirdness that happened when you were trying to merge in existing changes from main.
In the following screenshot, commits 1 and 2 shouldn't be there and 3 should be merging main into experimental-merge-of-designs.
image

I'm happy to help fix this if needed

theme: {
colors: {
background: "#b3b3b3",
// background: "#4d4d4d",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can get rid of this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just testing with a dark theme color. Something that can be looked at a later point in time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A lot of this CSS can be removed without changing the way the page looks. That can be done in a follow-up ticket though in order to move things forward.

Comment on lines +62 to +63
{/* </div>*/}
{/*</div>*/}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be removed

Comment on lines +10 to +13
<link
href="https://fonts.googleapis.com/css2?family=Inter:ital,opsz,wght@0,14..32,100..900;1,14..32,100..900&display=swap"
rel="stylesheet"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if we're even using this font. I deleted this locally and nothing changed.

@Cykotech Cykotech force-pushed the experimental-merge-of-designs branch from 1636ef2 to 411a11c Compare January 16, 2025 05:24
@Cykotech Cykotech merged commit e7009f8 into main Jan 16, 2025
3 checks passed
@Cykotech Cykotech deleted the experimental-merge-of-designs branch January 16, 2025 05:26
@aedwardg aedwardg mentioned this pull request Jan 23, 2025
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.

Page Design

4 participants