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

Applied most style changes from the new figma design #327

Merged
merged 11 commits into from
Oct 5, 2023

Conversation

Mink-Mink
Copy link

@Mink-Mink Mink-Mink commented Sep 29, 2023

You can see the figma stylesheet here https://www.figma.com/file/q7DDvvkSKfYpT0yheLexht/AI-Safety-ccc-ccstan99

  • Most of the changes are there
  • I didn't add a gray "tree" structure to the collapsable question sections because that seemed more complex
  • Also didn't move search bar into the header because it's unclear to me how that would interact with the tags page (do we put tag filtering there? Do we make logic so that if someone types a question in there they get switched back to the base page?)
  • Added a back button to the tags page - it wasn't in the design, but needing to edit the URL or click on stampy icon is really unintuitive when wanting to go back to the base question page without picking a tag
  • See comments below for screenshots of the new look
  • As you can see in the dialogue post, currently it highlights only a part of the row; this is down to how the form there is structured, but it seemed like a bit too much of a refactor for me to attempt right away.
  • Blank space, in case I remember something later.

Note from @Aprillion: Related to issue #285

@Mink-Mink
Copy link
Author

image

@Mink-Mink
Copy link
Author

image

@Mink-Mink
Copy link
Author

image

@Mink-Mink
Copy link
Author

image

@Mink-Mink
Copy link
Author

image

@Mink-Mink
Copy link
Author

image

@Mink-Mink
Copy link
Author

image

@Mink-Mink
Copy link
Author

image

@Mink-Mink
Copy link
Author

image

@Mink-Mink
Copy link
Author

image

Black border here is due to the screenshotting software, it's clean.

@Aprillion Aprillion mentioned this pull request Sep 29, 2023
@StampyAI StampyAI deleted a comment from Mink-Mink Sep 29, 2023
app/root.css Outdated
--bgColorHighlight: #c0d3e4;
--bgColorTableRows: #e8f7fe;
--colorLink: #79f;
--colorLinkVisited: #8ac;
--colorTooltip: #d1d1d1;
--colorTooltip: rgba(48, 115, 183, 0.1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • could you please find a better background for .answer .link-popup? transparent --colorTooltip is no good for that:
    image

@ccstan99 - I didn't find the popup styles in Figma - I'm happy if @Mink-Mink will pick from existing bg colors unless you have some new color in mind and just forgot to specify it yet

Copy link
Collaborator

Choose a reason for hiding this comment

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

No transparency please. No new color. Feel free to pick from the existing bg color.

{...props}
>
<path
className="tag_svg__gray"
Copy link
Collaborator

Choose a reason for hiding this comment

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

good try 👍 but this needs to be set in the SVG file itself as class="gray", not a manual change in generated file

I pushed a fix commit to your forked repo, please pull before making more changes, thanks!

@ccstan99
Copy link
Collaborator

ccstan99 commented Oct 1, 2023

A few other minor things:

  • "Show me even more (more) questions about AI Safety..." background should be transparent not darker gray.
    (Please fix typo for us: deduplicate "more more")
  • Question titles should be bolded to contrast from answers.
  • All tags border outline should be blue (same color as text) not gray.

If they can be easily incorporated, please also match Figma for

  • Size of tags text on tags page
  • "None of these: Request an answer to my exact question above" should be styled
  • Title of popup dialogues should be larger & bold
  • Please use Figma specified colors for background & foreground.
Screenshot 2023-09-30 at 4 51 51 PM Screenshot 2023-09-30 at 4 52 01 PM

@ccstan99
Copy link
Collaborator

ccstan99 commented Oct 1, 2023

Also, you were referring to a slightly out-of-date Figma file. Please use this for reference. Thanks!

Aprillion
Aprillion previously approved these changes Oct 1, 2023
Copy link
Collaborator

@Aprillion Aprillion left a comment

Choose a reason for hiding this comment

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

adding a few more color changes to match the Figma designs from #285 (only where I noticed some differences while I was trying to find color for the popups, now reusing --bgColorHighlight for both hover in search results and popups)

image

approving from code review perspective, we can merge if designers won't have any more major objections (minor fixes can be done later if we won't notice them right now)

deployed to https://stampy-ui.aprillion.workers.dev/

ccstan99
ccstan99 previously approved these changes Oct 1, 2023
Copy link
Collaborator

@ccstan99 ccstan99 left a comment

Choose a reason for hiding this comment

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

This is much improved, thank you! I'm happy for this to be approved & merged but let’s keep the original issue with link to Figma file open. I’ll add any items still outstanding to that issue.

@ccstan99
Copy link
Collaborator

ccstan99 commented Oct 1, 2023

One last glaring thing if we're able to sneak it easily into this PR... Can we please remove the border outline around "Show me even more questions about AI Safety..."? The highlight on hover is fine but there's no need to draw that much attention to it otherwise.

@Aprillion Aprillion dismissed stale reviews from ccstan99 and themself via e8bfe86 October 5, 2023 18:33
@Aprillion
Copy link
Collaborator

border removed, merging now and let's solve follow-up issues in future PRs
image

@Aprillion Aprillion merged commit 964d457 into StampyAI:master Oct 5, 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.

3 participants