Fix join communities design review bugs #17094
Merged
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
jo-mut
force-pushed
the
bug/join-community-reviews
branch
from
aef158d
to
eb6e5b1
Compare
Jenkins BuildsClick to see older builds (83)
|
jo-mut
force-pushed
the
bug/join-community-reviews
branch
2 times, most recently
from
3abd585
to
95e7701
Compare
jo-mut
requested review from
Parveshdhull,
J-Son89,
ibrkhalil,
ulisesmac and
smohamedjavid
jo-mut
force-pushed
the
bug/join-community-reviews
branch
from
95e7701
to
82589dd
Compare
ulisesmac
reviewed
Aug 23, 2023
Comment on lines
74
to
76
| :background-color (colors/theme-colors | ||
| colors/white | ||
| colors/neutral-95)}) |
There was a problem hiding this comment.
These params fit in the same line 👍
ulisesmac
approved these changes
Aug 23, 2023
jo-mut
force-pushed
the
bug/join-community-reviews
branch
from
82589dd
to
d2a70e8
Compare
J-Son89
reviewed
Aug 24, 2023
| {:border-radius picture-diameter | ||
| :width picture-diameter | ||
| :height picture-diameter | ||
| :background-color (colors/theme-colors colors/white colors/neutral-95)}) |
There was a problem hiding this comment.
can you pass theme as the 3rd arg here? 🙏
J-Son89
approved these changes
Aug 24, 2023
Parveshdhull
approved these changes
Aug 24, 2023
smohamedjavid
approved these changes
Aug 24, 2023
jo-mut
force-pushed
the
bug/join-community-reviews
branch
from
d2a70e8
to
3b4fe61
Compare
65% of end-end tests have passedFailed tests (15)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Passed tests (28)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
jo-mut
force-pushed
the
bug/join-community-reviews
branch
4 times, most recently
from
13fd69b
to
5f7c64f
Compare
jo-mut
force-pushed
the
bug/join-community-reviews
branch
from
d9937c3
to
5aa6f9a
Compare
|
@pavloburykh I suppose this pr then is ready for merge |
if @Francesca-G approves - then it is ready for merge |
Francesca-G
approved these changes
Sep 1, 2023
jo-mut
force-pushed
the
bug/join-community-reviews
branch
5 times, most recently
from
ebccbd8
to
7bc11f5
Compare
jo-mut
force-pushed
the
bug/join-community-reviews
branch
2 times, most recently
from
2a3bc72
to
1eae2d2
Compare
jo-mut
force-pushed
the
bug/join-community-reviews
branch
from
1eae2d2
to
bced936
Compare
|
Hey @jo-mut! After merging of this PR it turned out that there are couple of changes that were not expected by this PR:
As far as I see there is a separate PR on that which is review column. So I wonder how these changes leaked in this PR?
I don't see any mention in this PR about this change (correct me I am wrong).
When I tested the build from August 31 there were none of these changes. Also I don't see any requests from @Francesca-G regarding these changes in this PR. Am I missing something? One of the problems is that such unexpected changes which are silently merged into develop without manual qa approval (or at least running of e2e) can brake e2e tests or can potentially cause some other bugs. I see that community join flow has been updated in Figma But we should implement any updates according to our standard process: creating an issue with description, creating corresponding PR, reviewing the PR, passing e2e test, approving from QA team (if needed), passing design review. Maybe I have missed some conversations/agreements regarding those changes. Will appreciate if you clarify on that. Thank you! |
|
@pavloburykh The figma link above removed the checkbox from the community rules and the feeback figmas were updated like this without the checkbox |
|
As for the changes on join community button there is an issue for that and a pr. #17065. I may have leaked some of those changes into this pr which is my mistake |
|
it contradicts with another PR which was merged one week ago. cc @cammellos |
|
@churik with the updated figma there is no more |
|
@jo-mut anyway if drastic changes like this are made after design review, please, re-ask for manual QA, as currently we have all community e2e broken in develop. Thank you. |
|
thank you |
|
Hey @jo-mut! Also, I see "Follow up" label in this PR. If you have already created followup please link it to this PR. If not - please create one according to our process. Thank you. |
|
thats right. thanks |
This was referenced Sep 7, 2023
Closed
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.
fixes #16862