fix: update use of setting-item component to match new api #17244
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
J-Son89
requested review from
OmarBasem,
mohsen-ghafouri,
jo-mut and
siddarthkay
and removed request for
OmarBasem
J-Son89
commented
Sep 11, 2023
| @@ -62,7 +62,8 @@ | |||
| (case tag | |||
| :positive [status-tags/status-tag | |||
| {:status {:type :positive} | |||
| :label (i18n/label :t/positive) | |||
| :label (:label tag-props) | |||
There was a problem hiding this comment.
status tags use a custom label not just positive/ negative 👍
There was a problem hiding this comment.
@J-Son89 there is context tags, or is this something different?
There was a problem hiding this comment.
This text should be dynamic - that's how they are using it across the screens.
There was a problem hiding this comment.
Jenkins BuildsClick to see older builds (16)
|
jo-mut
approved these changes
Sep 11, 2023
65% of end-end tests have passedFailed tests (15)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (28)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
OmarBasem
approved these changes
Sep 13, 2023
churik
force-pushed
the
jc/syncing-page
branch
from
ab5084b
to
dc10500
Compare
84% of end-end tests have passedFailed tests (7)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (36)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
|
|
@J-Son89 thanks for your work! |
Francesca-G
approved these changes
Sep 15, 2023
I commented on the figma, there is a note about this in the description 👍 |
|
@Francesca-G - the component aligns correctly once the text is added. I added this issue for the backend implementation to be added so this "Next back up in ..." text can be added and everything sits right 👍 Going to merge this pr if that's okay? additionally another option would be to put an empty string in for the subtext i.e I will put " " there, this will mean the icon will align correctly but there. will be a gap between the tag and title then. |
J-Son89
force-pushed
the
jc/syncing-page
branch
from
dc10500
to
366a5ab
Compare
J-Son89
force-pushed
the
jc/syncing-page
branch
2 times, most recently
from
23abffc
to
59448e5
Compare
J-Son89
force-pushed
the
jc/syncing-page
branch
from
59448e5
to
2d09263
Compare
✔️ status-mobile/prs/ios/PR-17244#6 🔹 ~7 min 2 sec 🔹 2d09263 🔹 📦 ios package |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
follow up to: #17179
The setting-item component had some recent refactors to its api but the use of this component was not adjusted on the syncing page.
This fixes that.
To test, check the settings-item component and its uses on the syncing page.
Before:
After:
Note!
Unfortunately the designs for the syncing page include the text "Next backup in ...". Without this text the component will not sit right as in the designs. This text is not available and there are considerations of whether we actually want to retrieve this data (cc @cammellos)
So at the moment the component will not sit right unless we have that secondary text being used in the component.
e.g it would look like this:
