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

Fixes for highlights mobile #11209

Merged
merged 4 commits into from
Apr 1, 2022

Conversation

gabiborlea
Copy link
Contributor

@gabiborlea gabiborlea commented Mar 24, 2022

Use stacked buttons
Before
Screenshot 2022-03-24 at 14 30 22

After
Screenshot_1648720294

Fix get sessionId on mobile

@gabiborlea gabiborlea changed the title fix(hightlight): center buttons text in higtlight dialog mobile fix(hightlight): center buttons text in highlight dialog mobile Mar 24, 2022
@gabiborlea gabiborlea changed the title fix(hightlight): center buttons text in highlight dialog mobile fix(highlight): center buttons text in highlight dialog mobile Mar 24, 2022
@gabiborlea
Copy link
Contributor Author

@saghul added some screenshots

@saghul
Copy link
Member

saghul commented Mar 24, 2022

Buttons should be stacked vertically, that's the common mobile UX pattern.

Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

(switching to request changes to we adapt the UI)

flex: 1,
flexDirection: 'row',
justifyContent: 'center',
alignItems: 'center'
Copy link
Contributor

@horymury horymury Mar 25, 2022

Choose a reason for hiding this comment

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

think you could replace:
flexDirection: 'row', justifyContent: 'center', alignItems: 'center'

with : justifyContent: 'space-around'

@gabiborlea gabiborlea force-pushed the gborlea/fix-highlight-mobile branch from 060e845 to 303359b Compare March 31, 2022 08:58
@gabiborlea gabiborlea changed the title fix(highlight): center buttons text in highlight dialog mobile Fixes for highlights Mar 31, 2022
@gabiborlea gabiborlea force-pushed the gborlea/fix-highlight-mobile branch from d575abd to c5a3a4b Compare March 31, 2022 14:00
fix(highlights): switch buttons order

fix(highlights): remove unused logger import
@gabiborlea gabiborlea force-pushed the gborlea/fix-highlight-mobile branch from c5a3a4b to 73935d8 Compare March 31, 2022 14:04
@gabiborlea gabiborlea changed the title Fixes for highlights Fixes for highlights mobile Mar 31, 2022
@gabiborlea gabiborlea requested a review from saghul March 31, 2022 14:08
saghul
saghul previously approved these changes Apr 1, 2022
@@ -215,7 +215,7 @@ export async function sendMeetingHighlight(state: Object) {

const reqBody = {
meetingFqn: extractFqnFromPath(state),
sessionId: conference.sessionId,
sessionId: conference.sessionId || conference.room.meetingId,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saghul is this a good way to get the sessionId on mobile?

Copy link
Member

Choose a reason for hiding this comment

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

Let me check.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sessionId: conference.sessionId || conference.room.meetingId,
sessionId: conference.getMeetingUniqueId(),

@saghul saghul merged commit 1e58a7c into jitsi:master Apr 1, 2022
pull bot pushed a commit to e4basil/jitsi-meet that referenced this pull request Apr 1, 2022
tmoldovan8x8 pushed a commit that referenced this pull request Apr 5, 2022
ankit-programmer pushed a commit to ankit-programmer/jitsi-meet that referenced this pull request May 7, 2022
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.

3 participants