StampyAI / stampy-ui

AI Safety Q&A web frontend
https://aisafety.info
MIT License
35 stars 9 forks source link

categories page #379

Closed melissasamworth closed 7 months ago

melissasamworth commented 8 months ago

Update (12 Feb) Almost done, just a few small styling issues (see comment below)

Acceptance Criteria

jrhender commented 7 months ago

@melissasamworth @mruwnik for the article page, issue #382 asks that the links be "aisafety.com/id/current-title-of-article". Would it make sense to also have the category URLs have the name and the id? In the current implementation, the category URLs are /tags/{tag name}. Would it instead make sense to do /tags/{tag id}/{tag name}, similar to the articles? EDIT: done here https://github.com/StampyAI/stampy-ui/pull/384

jrhender commented 7 months ago

This tasks probably isn't quite done from a styling standpoint as the sidebar doesn't quite match figma:

Image

melissasamworth commented 7 months ago

Hey @jrhender quick process note: do you think it's best to talk about individual tickets here on Github? I think it probably does, and if so I'll make sure to check this daily (sorry for the lag this time).

The purpose of the IDs with articles is to make it backwards compatible, particularly for links people have shared that are floating out there. I'm not sure how many tag links people have shared. Still, backwards compatibility would be nice. Does your solution solve that problem?

Re styling: There are some issues, but I am not sure they fit under the scope of this task.

Those styling issues notwithstanding, this is my QA before passing things off to done column

Image

jrhender commented 7 months ago

The search input current says "Search articles" but it should say "Filter by keyword"

PR here: https://github.com/StampyAI/stampy-ui/pull/391

jrhender commented 7 months ago

Lastly, not sure if it should belong to this issue but the widths of the layout is off. Ideally they should be the following viewport widths

@melissasamworth @buddy-web3

Do you think the change to fix this would be in newRoot.css? https://github.com/StampyAI/stampy-ui/blob/04b76d21a98c0470814a78260a896f8f1e359f59/app/newRoot.css#L408

melissasamworth commented 7 months ago

Storybook version pushed Still needs to be integrated with article page (Nemo)

jrhender commented 7 months ago

Hey @melissasamworth , could you elaborate on this point please:

Storybook version pushed

The storybook version of one of the components was published? By "published" do you mean push to the stampy-redesign branch?

melissasamworth commented 7 months ago

Lastly, not sure if it should belong to this issue but the widths of the layout is off. Ideally they should be the following viewport widths

@melissasamworth @buddy-web3

Do you think the change to fix this would be in newRoot.css?

stampy-ui/app/newRoot.css

Line 408 in 04b76d2

.group-elements {

Not sure but i can take a look soon

melissasamworth commented 7 months ago

Hey @melissasamworth , could you elaborate on this point please:

Storybook version pushed

The storybook version of one of the components was published? By "published" do you mean push to the stampy-redesign branch?

@joy-void-joy Hey I'm sorry, I think I meant to comment this on another issue.

Only thing remaining is "If no article title takes up 2 lines, can you still make them 100% width?" thanks :)

jrhender commented 7 months ago

@melissasamworth can you explain this point some more please?

Only thing remaining is "If no article title takes up 2 lines, can you still make them 100% width?" thanks :)

I think what you might be saying is that you want all of the article boxes to have at least the height of a 2-line title article box. Is this right? For example, this screenshot shows that 2-line article title boxes have more height than a 1-line article title box:

image
melissasamworth commented 7 months ago

@jrhender I was referring to the widths, but don't worry, Nemo took care of it

jrhender commented 7 months ago

Awesome, thanks for taking this across the finish line @mruwnik and @melissasamworth !