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

info panel not modal #21990

Merged
merged 8 commits into from Jan 16, 2020
Merged

info panel not modal #21990

merged 8 commits into from Jan 16, 2020

Conversation

chrisnojima
Copy link
Contributor

@chrisnojima chrisnojima commented Jan 10, 2020

  • changes info panel to not be a modal anymore on desktop

note: there is a known issue where if you click on the info panel, click an image, then click all media, then click back the attachments are gone. how state is managed and how the info panel (in general) is organized is a huge mess and needs to be refactored

@@ -3534,6 +3520,26 @@ const refreshBotSettings = async (_: Container.TypedState, action: Chat2Gen.Refr
return Chat2Gen.createSetBotSettings({conversationIDKey, settings, username})
}

const onShowInfoPanel = (action: Chat2Gen.ShowInfoPanelPayload) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mobile only pushes/pops the route

@@ -1,37 +0,0 @@
import * as React from 'react'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file basically did nothing, so hoisting the child up a level and replacing it

)
const conversationIDKey: Types.ConversationIDKey =
// @ts-ignore
typeof ownProps.navigation !== 'undefined'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not bothering to fix these types as this needs to be cleaned up majorly

<Conversation navigation={props.navigation} />
{infoPanelShowing && <InfoPanel />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

InfoPanel lives here now

@@ -18,7 +18,8 @@ export const isDeviceSecureAndroid: boolean =
: nativeBridge.isDeviceSecure === 'true' || false
export const isTestDevice = nativeBridge.isTestDevice

export const isRemoteDebuggerAttached = typeof __REMOTEDEV__ !== 'undefined'
// @ts-ignore
export const isRemoteDebuggerAttached: boolean = typeof DedicatedWorkerGlobalScope !== 'undefined'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this never worked on ios, and likely never worked on android

@@ -194,6 +194,8 @@ export type State = Readonly<{
inboxLayout: RPCChatTypes.UIInboxLayout | null // layout of the inbox
inboxSearch?: InboxSearchInfo
inboxShowNew: boolean // mark search as new,
infoPanelShowing: boolean
infoPanelSelectedTab: 'settings' | 'members' | 'attachments' | 'bots' | undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

have to keep track of this in the store for desktop now

@chrisnojima chrisnojima changed the title WIP: info panel not modal info panel not modal Jan 10, 2020
@chrisnojima chrisnojima requested review from mmaxim and a team January 10, 2020 21:51
@chrisnojima chrisnojima marked this pull request as ready for review January 10, 2020 21:51
Copy link
Contributor

@mmaxim mmaxim left a comment

Choose a reason for hiding this comment

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

Seems way too wide on desktop, is that intended?

@adamjspooner
Copy link
Contributor

Agreed (on the width).

@chrisnojima
Copy link
Contributor Author

Probably was constrained by the animation container before. Likely a 1 liner

@mmaxim
Copy link
Contributor

mmaxim commented Jan 11, 2020

Behavior is a little weird if you are on a "members" tab from a team, then land on an imp team, since it doesn't have that tab.

@mmaxim
Copy link
Contributor

mmaxim commented Jan 11, 2020

I think the best fix for that Membes tab situation is to actually maintain two saved tabs, one for teams and one for ad hoc. Since if you just say "if current tab members, and on ad hoc conv, set to attachments" you keep switching the team convs over to attachments, even if you want it to stay on members.

@adamjspooner might have an opinion here.

@adamjspooner
Copy link
Contributor

Seems okay to me. 👍

@mmaxim
Copy link
Contributor

mmaxim commented Jan 11, 2020

We can maybe move this to another PR, but I think this thing needs some kind of loading state when entering a conv. It was less of a problem before since it wasn't likely to click into a conv and immediately hit the info panel, but now if you go into a conv with a ton of people, and you have the info panel open, you just look at a blank state with no spinner while we churn away loading everything.

@chrisnojima
Copy link
Contributor Author

As mentioned here how this works internally needs attention. I think we should do it in another pr. It can happen this sprint

@chrisnojima
Copy link
Contributor Author

@mmaxim we ok to merge this

@mmaxim
Copy link
Contributor

mmaxim commented Jan 13, 2020

Fine to merge, but we need to fix that Members tab thing before release.

@chrisnojima
Copy link
Contributor Author

@mmaxim merging when ci passes

@chrisnojima chrisnojima merged commit bbf5f21 into master Jan 16, 2020
@chrisnojima chrisnojima deleted the nojima/HOTPOT-move-info-1 branch January 16, 2020 18:01
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.

None yet

3 participants