-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add filter to homepage #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the filter seems to be working properly! just a couple of questions about some things
flex-direction: column; | ||
height: 500px; | ||
justify-content: space-between; | ||
min-width: 350px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we want the css properties to be alphabetized?
if (displayMyStories) { | ||
return storyTranslationId ? "review book" : "translate book"; | ||
} | ||
return "view translation"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the user presses a button with view translation should that also try to assign them as either a reviewer/translator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I think some of our existing logic is flawed here.
If the user is browsing stories/translations, the call-to-action-button (primary button) should assign them as translator/reviewer.
If the user is looking at story translations they are already translators/reviewers of, the call-to-action-button (primary button) should open the translation platform.
logic is now changed to reflect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we toggle for role "For translation", should only the "translate book" button show up? or can the "review book" button show up as well?
right now when i'm toggled to For Translation i see only the review book button, but when i toggle to For Review i see nothing (could also be my test data so i'll keep testing but wanted to clarify)
onToggle={setRole} | ||
/> | ||
</div> | ||
<div id="filter-level"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a change, hows it look now?
slider doesnt have a figma design right now - Russel and I discussed using sliders over buttons on slack. Currently the slider's styling is default html/css.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -134,16 +144,16 @@ const StoryCard = ({ | |||
<p className="story-card-title">{title}</p> | |||
<div className="story-card-tags"> | |||
{/* TODO: make tags that reflect actual state */} | |||
<Tag displayText={`level ${level}`} /> | |||
<Tag displayText="language needed" /> | |||
<Tag displayText={`Level ${level}`} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did design have any preference for capitalization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Will double-check with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The width is awkward on my laptop, so it probably looks awkward for others as well
- Can we change "Filter Stories" so it looks more like a title? The categories are caps and the title is lowercase, with very similar sizes. I almost expect there to be another filter setting underneath Filter stories
- Role toggle button looks weird. Probably some CSS fun to sort out
One more thought - what components within the HomePage
directory are re-usable? Looking at the parts there, I don't think the primary/secondary buttons are necessarily homepage-specific. Along with the toggle button, they might deserve their own components folder
<div className="filter"> | ||
<h1>Filter Stories</h1> | ||
<div> | ||
<h4>TRANSLATION LANGUAGE</h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we establish some standards around what font sizes/styles correspond to h1
and h4
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
role, | ||
setRole, | ||
}: FilterProps) => { | ||
// language state in Homepage can't default to user context value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't homepage handle the default logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 i think i was hallucinating or something. Works now, logic moved to homepage
}: FilterProps) => { | ||
// language state in Homepage can't default to user context value | ||
// set the default language from user approvedLanguages here | ||
if (!Object.keys(approvedLanguages).includes(language)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does !(language in approvedLanguages)
not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(code is outdated now) but yee that works too!
@@ -58,6 +59,7 @@ const StoryCard = ({ | |||
youtubeLink, | |||
level, | |||
language, | |||
displayMyStories, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: is there another name that we could use? displayMyStories
feels like a homepage-level logic concept, that seems distinct from what the storyCard
component should know
- maybe
isAssignedToUser
,storyInProgress
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of isMyStory
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, don't mind that at all
level: number, | ||
userId: number, | ||
): QueryInformation => { | ||
let result = {}; | ||
|
||
if (role === Role.Translator && !displayMyStories) { | ||
if (isTranslator && !displayMyStories) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh nice this is so much prettier, thank you!!
@e-wai reverted toggle button css changes, lmk how it looks. Regarding reusability - yeah, a ton of homepage files are reusable. In a separate PR I want to split off a lot of stuff into a utils/ folder. This will also be necessary for design-system-y-stuff (eg consistent Titles and Subtitles) Regarding Filter title: I'll keep it as it is for now as its inline with figma designs, but this is good feedback for design that we can discuss at Sundays meeting. |
</div> | ||
<div> | ||
<h4>ROLE</h4> | ||
<ToggleButton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also comment for general interest - the escape character for apostrophe was double single quote for me (
|
Toggle & header/story card css alignment issues should be good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toggle button all nice and pretty now! @gaoxk created an issue for CSS troubleshooting homepage 🚀
Everything else looks good to me 🎉
role + displayMyStory + button text combo was incorrect. button text logic was backwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
Notion ticket link
add filter
Implementation description
note:
story
folder tohomepage
Steps to test
'{"HINDI":3, "FRENCH":2, "BENGALI":4}'
. make a second user, but log in as the first user.TODO: grey out filter on viewing my work
Checklist
docker exec -it <backend-container-id> /bin/bash -c "black . && isort --profile black ."