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

#4642 Update the top bar in a room #5213

Merged
merged 6 commits into from
Mar 4, 2022
Merged

#4642 Update the top bar in a room #5213

merged 6 commits into from
Mar 4, 2022

Conversation

ahmed-radhouane
Copy link
Contributor

@ahmed-radhouane ahmed-radhouane commented Feb 11, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Motivation and context

  • Update the Shield and the ON/OFF notification within the Room toolbar image.
  • Remove the subtitle
  • reduce extra padding for the menu item icons.

Screenshots / GIFs

Dark Light
Capture d’écran 2022-02-23 à 17 31 24 Capture d’écran 2022-02-23 à 17 31 30

Tests

  • Access to a timeline and check the toolbar.

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@ahmed-radhouane ahmed-radhouane marked this pull request as draft February 11, 2022 17:31
@github-actions
Copy link

github-actions bot commented Feb 11, 2022

Unit Test Results

  92 files  ±0    92 suites  ±0   1m 13s ⏱️ +4s
162 tests ±0  162 ✔️ ±0  0 💤 ±0  0 ±0 
524 runs  ±0  524 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 0f3e42a. ± Comparison against base commit 6e6b04c.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Feb 11, 2022

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed=
  • [org.matrix.android.sdk.account]
    passed=
  • [org.matrix.android.sdk.internal]
    passed=
  • [org.matrix.android.sdk.ordering]
    passed=
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed=

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

I have seen that the PR is draft, but some first remarks.

setTypeface(null, Typeface.BOLD)
}
setTextOrHide(topic)
setTextColor(colorProvider.getColorFromAttribute(R.attr.colorPrimary))
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to keep vctr_content_secondary here. Or better declare it in the XML since it will not change anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Thanks

}
setTextOrHide(topic)
setTextColor(colorProvider.getColorFromAttribute(R.attr.colorPrimary))
setTypeface(null, Typeface.BOLD)
Copy link
Member

Choose a reason for hiding this comment

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

Typeface.NORMAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Thanks

private fun renderSubTitle(typingMessage: String?, topic: String) {
// TODO Temporary place to put typing data
val subtitle = typingMessage?.takeIf { it.isNotBlank() } ?: topic
private fun renderSubTitle(topic: String) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the aim of #4642 is to completely remove the subtitle, so stop displaying the room topic there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Thanks

@ahmed-radhouane ahmed-radhouane marked this pull request as ready for review February 23, 2022 17:34
@ahmed-radhouane ahmed-radhouane changed the base branch from arb/issues/3296 to develop February 23, 2022 17:38
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks. Some remarks

@@ -0,0 +1 @@
Update the top bar in a room
Copy link
Member

Choose a reason for hiding this comment

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

I would give more details:

Suggested change
Update the top bar in a room
Update the top bar in a room: remove topic and typing information

@@ -25,4 +25,10 @@
<item name="android:backgroundDimEnabled">false</item>
</style>

<style name="Theme.Vector.ActionButton" parent="@android:style/Widget.ActionButton">
Copy link
Member

Choose a reason for hiding this comment

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

This is not the correct location to add this new style. Can you move it to a new file style_action_button.xml in the same folder please?

@@ -25,4 +25,10 @@
<item name="android:backgroundDimEnabled">false</item>
</style>

<style name="Theme.Vector.ActionButton" parent="@android:style/Widget.ActionButton">
Copy link
Member

Choose a reason for hiding this comment

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

Please rename the style to Widget.Vector.ActionButton

@@ -25,4 +25,10 @@
<item name="android:backgroundDimEnabled">false</item>
</style>

<style name="Theme.Vector.ActionButton" parent="@android:style/Widget.ActionButton">
Copy link
Member

Choose a reason for hiding this comment

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

The parent should be Widget.AppCompat.ActionButton I think

</style>



Copy link
Member

Choose a reason for hiding this comment

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

Please remove those extra lines.

app:layout_constraintTop_toTopOf="@id/roomToolbarTitleView" />
android:layout_width="11dp"
android:layout_height="13dp"
tools:ignore="MissingConstraints"
Copy link
Member

Choose a reason for hiding this comment

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

Please format the file. tools attribute has to be the latest attribute. You can type Maj+Option+Cmd+L in Android Studio to check the box to rearrange code.

android:layout_marginStart="7dp"
android:layout_marginEnd="8dp"
android:layout_marginStart="12dp"
android:layout_marginEnd="12dp"
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just delete this View (with id roomToolbarSubtitleView), it's always hidden now, so it's dead code.

 - remove typing message notification from room toolbar.

Signed-off-by: Ahmed Radhouane Belkilani <[email protected]>
 - update image and badge in room toolbar.

Signed-off-by: Ahmed Radhouane Belkilani <[email protected]>
- remove subtitle in room toolbar.
- update shiled and connected icon in toolbar main picture.

Signed-off-by: Ahmed Radhouane Belkilani <[email protected]>
- reduce extra padding between menu item.

Signed-off-by: Ahmed Radhouane Belkilani <[email protected]>
- adding the changelog file.

Signed-off-by: Ahmed Radhouane Belkilani <[email protected]>
 - Fixing after code review.

Signed-off-by: Ahmed Radhouane Belkilani <[email protected]>
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I will squash and merge.

@bmarty bmarty merged commit bcdf004 into develop Mar 4, 2022
@bmarty bmarty deleted the arb/issues/4642 branch March 4, 2022 15:38
onurays added a commit that referenced this pull request Mar 8, 2022
* develop: (392 commits)
  Missing import of at-Ignore annotation.
  FTUE - Choose a display picture (#5323)
  Ignore ThreadMessagingTest as it seems to cause other integration tests to fail.
  Maybe the file is here?
  I give up for the weekend
  Frustration at artifact handling vs what's in docs.
  Update the top bar in a room (#5213)
  Tweak upload/download of codecov xml file
  Address review points from adam
  Remove unneeded code, retaining a comment for how to exclude certain projects
  Merge pull request #5405 from vector-im/cgizard/ISSUE-5402
  Remove the printing of file name to the log as it's doubling up information.
  Remove exclusions (for now).
  Fix typo in name of action
  giving avatar/display name error dialogs human readable error messages - reuses the ErrorDialog logic which translates exceptions to human readable strings
  Run codecoverage and pass to sonarqube upload for processing.
  Better codecov based on ouchadam's suggestion
  Correct name of environment variable
  Use environment variable that is tied to project property
  Merge pull request #4498 from vector-im/yostyle/fix_strandhogg
  ...

# Conflicts:
#	vector/src/main/java/im/vector/app/features/home/room/detail/timeline/action/MessageActionsViewModel.kt
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.

2 participants