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

Update LHN layout in GSD Priority Mode #1914

Closed
NicMendonca opened this issue Mar 19, 2021 · 5 comments · Fixed by #2015
Closed

Update LHN layout in GSD Priority Mode #1914

NicMendonca opened this issue Mar 19, 2021 · 5 comments · Fixed by #2015
Assignees

Comments

@NicMendonca
Copy link
Contributor

NicMendonca commented Mar 19, 2021

If you haven’t already, check out our contributing guidelines for onboarding!


Expected Result:

  • For the rows to be tightly stacked, and compact when in "GSD" Priority Mode. The avatars should be smaller and the distance between names/rooms should be less than they currently are in the "Most Recent" Priority Mode:

image

Actual Result:

  • Layout for GSD is the same as "Most Recent" (not compact)

image

Action Performed:

  • On Desktop, click your avatar at the top to show Settings on right side
  • Click Preferences
  • Select the Priority Mode drop down menu then enable GSD
  • Observe LHN (left hand navigation)

Workaround:

N/A (not a bug, this is an improvement)

Platform:

All:

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Notes/Photos/Videos: Any additional supporting documentation
Above 👆

Job posting: https://www.upwork.com/jobs/~0164a6dae10c0e9380

@parasharrajat
Copy link
Member

parasharrajat commented Mar 19, 2021

Hi @NicMendonca , I would like to work on this issue, I have already worked on a similar issue that requires adding a tooltip to the Chat title users' names.

Proposal

  1. Take the priority mode settings from Onyx store. Which is already present on the SidebarLinks.js.
  priorityMode: {
            key: ONYXKEYS.PRIORITY_MODE,
        },
  1. Now I see that OptionRow component states less and all of the props are being passed from the parent Component. So I will pass down the priorityMode from the SidebarLinks.js component to the OptionsList and thus to the OptionRow.
    Let me know if are allowed to access the Onyx key directly over OptionRow.
    https://github.com/Expensify/Expensify.cash/blob/c220b1872934492fd6b0e9777b223658a003a8ad/src/components/OptionsList.js#L141-L146

  2. For avatars, in MultipleAvatars we pass a prop something named like size: 'small' based on priorityMode === CONST.PRIORITY_MODE.GSD the whose default value would be default.
    https://github.com/Expensify/Expensify.cash/blob/c220b1872934492fd6b0e9777b223658a003a8ad/src/pages/home/sidebar/OptionRow.js#L101-L104

  3. Now based on the size prop we set the styles on the Images in the MultipleAvatars.

  4. Update the OptionRow.js code
    https://github.com/Expensify/Expensify.cash/blob/c220b1872934492fd6b0e9777b223658a003a8ad/src/pages/home/sidebar/OptionRow.js#L107-L199
    Like this when we are inpriorityMode === CONST.PRIORITY_MODE.GSD

<View style={[styles.flex1, styles.flexRow, {
                              overflow: 'hidden',
                          }]}
                          >
                              <Text
                                  style={[styles.optionDisplayName, textUnreadStyle, {
                                      minWidth: 'auto',
                                      flexBasis: 'auto',
                                      flexGrow: 0,
                                      flexShrink: 0,
                                  }]}
                                  numberOfLines={1}
                              >
                                  {option.text}
                              </Text>
                              {option.alternateText ? (
                                  <Text
                                      style={[textStyle, styles.optionAlternateText, {
                                          flexShrink: 1,
                                          flexGrow: 1,
                                          flexBasis: 'auto',
                                      }]}
                                      numberOfLines={1}
                                  >
                                      {option.alternateText}
                                  </Text>
                              ) : null}
                          </View>

Which allows them to be in a single line but keep the chat title fully visible until the container end reaches.

Avatar size is yet to be decided after discussion


Inline-styles are just used for showing the difference

Please share the sizes for the avatars.

Questions/Observations.

  1. In the Actual result, I see the GSD Mode is shown compact. But in the master branch, it is not compact. So I have also added it into my proposal.

@flodnv
Copy link
Contributor

flodnv commented Mar 23, 2021

Hi @parasharrajat, your proposal sounds good, go ahead! One small comment with regards to (3): perhaps we would just pass the desired size in px to the prop, rather than default|small ?

Avatar size is yet to be decided after discussion

@Expensify/design what should the exact size be?

@flodnv flodnv assigned parasharrajat and unassigned flodnv Mar 23, 2021
@shawnborton
Copy link
Contributor

The mockups shown in the original comment are the exact sizes we need. We can provide a Figma file and help out with the PR review as well.

@parasharrajat
Copy link
Member

Ok Thanks. We will clear the sizes on the PR.

perhaps we would just pass the desired size in px to the prop, rather than default|small ?
There are few things which I took into consideration before deciding this:

  1. In old issues, we followed the same pattern for Header Component.
  2. I also think that we could have maximum three standard sizes like small | default | large for the Avatar on the OptionRow.
  3. Also, I have realized that we are trying to keep all the styling out of the JSX, thus using the classes would be good. Would like to hear from @roryabraham here.

Thanks.

@roryabraham
Copy link
Contributor

Let me know if are allowed to access the Onyx key directly over OptionRow

Yeah, I think you should use the withOnyx HOC to provide the OptionRow component with the the priorityMode.

With regards to (3), I think I agree with the original proposed solution, because MultipleAvatars has it's own internal logic for sizing and spacing the individual avatars, and it might be complex to make that work generically for any pixel value, when we only need it to work for two.

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 a pull request may close this issue.

5 participants