-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
MM-38682: Gekidou Homepage (Static) #5756
Conversation
Note: This depends on #5718 |
@SHAZM dependency merged, please resolve conflicts |
Adding @Rina-dsg for ux review as she's owning the mobile v2 UX from this point forward. I will take a look once Marina has reviewed though |
a463690
to
b7ef07e
Compare
Rebased and pushed. |
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.
Just a few comments from my side. Each PR has to be tested against mobile + tablets and ran against Android and iOS.
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.
In general looks good, just a few comments here and there. I see there are still some missing pieces, so I will refrain to approve until everything is in place.
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.
Some more comments from my side. Mostly nitpicks.
app/components/button/index.tsx
Outdated
|
||
const theme = useTheme(); | ||
|
||
const buttonStyles = [ |
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.
Should we memo these somehow to avoid rerenders? My first approach would be a state variable that is updated with useEffect.
@SHAZM is it ready for UX review? |
Thanks @SHAZM looks good. Thanks for adding the chevron to the team name and the other updates. Two comments:
|
Building app in separate branch. |
Updated -
|
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.
@SHAZM thanks for resolving these things. Looks good to me! I might need to view some things again when everything is hooked up, but I think this is in great shape to merge
Yes we are - I think one I'll do separately but I'll add a |
The status bar should be already resolved, have you merged the latest gekidou branch? |
a6ffbf0
to
7f8a03c
Compare
Just did - had a few conflicts and changes to run through but it all looks good 👍 |
@SHAZM still some conflicts... will start the review now |
app/components/channel_list/categories/body/channel/index.test.tsx
Outdated
Show resolved
Hide resolved
<CompassIcon | ||
size={24} | ||
name='server-variant' | ||
color={changeOpacity(theme.buttonColor, 0.56)} |
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 try this with the Quartz theme, I'm not sure this color fits that theme based on my previous tests switching themes.
FYI: @Rina-dsg
app/utils/buttonStyles.ts
Outdated
@@ -104,15 +105,23 @@ export const buttonBackgroundStyle = ( | |||
disabled: { | |||
default: { | |||
backgroundColor: changeOpacity(theme.centerChannelColor, 0.08), | |||
borderColor: changeOpacity(theme.centerChannelColor, 0.32), |
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 couldn't add a comment on line 68, but I believe primary buttons do not have a border even when they are focused, is that right @matthewbirtch ?
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.
Whoops no - you're right; I think this was a copy/paste fail for the disabled state - I'll fix up. The Primary/Default/Focus does have a border in the Figma file: https://www.figma.com/file/2uYWxjnMJ9IQDOba9b9bfV/%E2%9C%85Components---Buttons?node-id=1%3A184
I've gone through the review / feedback:
|
Summary
Very simple, static components for the Channel List screen. These are the barebones and will be developed further in subsequent PR's.
Highlights:
<Button emphasis='tertiary' size='lg' text='A Button' onPress=( () => Alert.alert('hi!')) />
<Text style={typography('Body', 200)}> ...
To-Do:
gekidou
Ticket Link
https://mattermost.atlassian.net/browse/MM-38682
Checklist
Device Information
This PR was tested on:
Screenshots
Release Note