Page MenuHomePhabricator
Differential D3839

[web] [remove] [ENG-1022] add long arrow and implement in sidebar
ClosedPublic
Actions

Authored by • benschac on Apr 25 2022, 11:25 AM.
Tags
None
Referenced Files
F5309492: D3839.id11983.diff
Tue, Apr 8, 6:57 AM
Unknown Object (File)
Mon, Apr 7, 12:29 AM
Unknown Object (File)
Mon, Apr 7, 12:27 AM
Unknown Object (File)
Mon, Apr 7, 12:25 AM
Unknown Object (File)
Mon, Apr 7, 12:23 AM
Unknown Object (File)
Mon, Apr 7, 12:21 AM
Unknown Object (File)
Mon, Apr 7, 12:19 AM
Unknown Object (File)
Mon, Apr 7, 12:17 AM
View All Files
Subscribers
• adrian
ashoat

Details

Reviewers
tomek
• jacek
• abosh
atul
Commits
rCOMM42d15674e084: [web] [remove] [ENG-1022] add long arrow and implement in sidebar
Summary

remove current implementation, that's the icomoon icons + border-left extensions.
Add -- a longer svg arrow for Chat TheadItems that have more than one sidebar.

This is just to add the second arrow for a Chat Thread Item with more than one sidebar.

image.png (798×916 px, 72 KB)

Note:

  • ENG-1053 added updated arrow here
  • I still need to update the ChatThreadItem styling a bit outlined here: ENG-1073
Test Plan

Icons should be completely removed. The design is pretty close to figma: https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1170%3A79456

The ChatThreadItems height needs to be adjusted a bit more.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• benschac created this revision.Apr 25 2022, 11:25 AM
Herald added subscribers: • karol, • adrian, ashoat. · View Herald TranscriptApr 25 2022, 11:25 AM
• benschac edited the summary of this revision. (Show Details)Apr 25 2022, 11:26 AM
Harbormaster completed remote builds in B8526: Diff 11885.Apr 25 2022, 11:30 AM
• benschac requested review of this revision.Apr 25 2022, 11:30 AM
• benschac added a child revision: D3841: [web, keyserver] [fix] [ENG-1022] add arrows much closer to spec [squashed diff into another stack].Apr 25 2022, 11:58 AM
tomek requested changes to this revision.Apr 27 2022, 6:29 AM

This diffs structure doesn't look like a good idea to me. We're removing some code here just to add it in the next diff. Maybe in this case merging the diffs makes the most sense?

web/chat/sidebar-item.react.js
19 ↗(On Diff #11885)

I don't think it is useful to remove this in this diff and add again in the next

This revision now requires changes to proceed.Apr 27 2022, 6:29 AM
• benschac updated this revision to Diff 11983.Apr 27 2022, 7:16 AM

squash commit

Harbormaster completed remote builds in B8599: Diff 11983.Apr 27 2022, 7:21 AM
• benschac added a comment.Apr 27 2022, 7:22 AM

Replying to @palys-swm's comments from D3841 since squashing the two diffs seemed like an easier review option. Closing D3841 in favor of this diff.

keyserver/images/arrow.svg
1 ↗(On Diff #11983)

From D3841

Why do we define xml version in one file and not in the other?

That's from @atul's sketch program. I don't think we need it. I'll remove it.

web/chat/sidebar-item.react.js
33 ↗(On Diff #11983)

D3841

I don't think this will be displayed, but if alt is visible, I think it would be better to be e.g. > or -> instead of this long text

I don't alt text is for screen readers. A blind person is going to want the image read to them so they know what it is. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img

• benschac planned changes to this revision.Apr 27 2022, 7:22 AM
• benschac mentioned this in D3841: [web, keyserver] [fix] [ENG-1022] add arrows much closer to spec [squashed diff into another stack].
• benschac added a comment.Apr 27 2022, 7:24 AM

Last question from the D3841

In D3841#107354, @palys-swm wrote:

I need to adjust the size of the chat thread item in another diff.
Are you sure we should do that?

Yes, it's a couple of pixels off and it's messing with the layout of the chat thread items.

• benschac updated this revision to Diff 12044.Apr 28 2022, 7:24 AM

remove xml tag

Harbormaster completed remote builds in B8653: Diff 12044.Apr 28 2022, 7:29 AM
tomek accepted this revision.Apr 29 2022, 5:20 AM

Please update the title, summary and test plan

web/chat/sidebar-item.react.js
33 ↗(On Diff #11983)

Ok, that makes sense. But I'm also wondering: if we care about accessibility and users with screen readers, shouldn't we test our app on a screen reader?

This revision is now accepted and ready to land.Apr 29 2022, 5:20 AM
tomek added 1 blocking reviewer(s): • jacek.Apr 29 2022, 5:22 AM

Adding @def-au1t as a blocking reviewer, so that he can verify if summary and test plan make sense after they're updated

This revision now requires review to proceed.Apr 29 2022, 5:22 AM
• benschac planned changes to this revision.May 2 2022, 8:39 AM
• benschac retitled this revision from [web] [remove] [ENG-1022] remove arrow and extender from sidebar to [web] [remove] [ENG-1022] add long arrow and implement in sidebar.
• benschac edited the summary of this revision. (Show Details)

I got a 2px shorter arrow from @atul. I need to update the img in the keyserver directory.

• benschac updated this revision to Diff 12136.May 2 2022, 9:35 AM

update svg

• benschac edited the summary of this revision. (Show Details)May 2 2022, 9:38 AM
• benschac planned changes to this revision.May 2 2022, 9:41 AM

this needs a little more work.

Harbormaster completed remote builds in B8715: Diff 12136.May 2 2022, 9:42 AM
• benschac updated this revision to Diff 12137.May 2 2022, 9:53 AM
• benschac edited the summary of this revision. (Show Details)

fixup arrows

• benschac edited the summary of this revision. (Show Details)May 2 2022, 9:53 AM
Harbormaster completed remote builds in B8716: Diff 12137.May 2 2022, 9:59 AM
• benschac edited the summary of this revision. (Show Details)May 2 2022, 10:02 AM
• benschac mentioned this in D3878: [web] [remove] [ENG-530] remove arrow from inline reply.May 2 2022, 2:45 PM
atul resigned from this revision.May 5 2022, 10:38 AM

(cleaning queue, looks like two other reviewers have/are taking a look)

• jacek requested changes to this revision.May 9 2022, 3:15 AM

I feel like the test plan still hasn't been updated. Am I right?

This revision now requires changes to proceed.May 9 2022, 3:15 AM
• karol removed a subscriber: • karol.May 9 2022, 8:04 AM
• benschac edited the summary of this revision. (Show Details)May 9 2022, 9:42 AM
• benschac edited the test plan for this revision. (Show Details)
• benschac edited the test plan for this revision. (Show Details)
In D3839#110920, @def-au1t wrote:

I feel like the test plan still hasn't been updated. Am I right?

You're right. I missed it. Updated the test plan.

• benschac requested review of this revision.May 9 2022, 9:43 AM
• jacek accepted this revision.May 10 2022, 1:35 AM
This revision is now accepted and ready to land.May 10 2022, 1:35 AM
Closed by commit rCOMM42d15674e084: [web] [remove] [ENG-1022] add long arrow and implement in sidebar (authored by • benschac). · Explain WhyMay 12 2022, 11:23 AM
This revision was automatically updated to reflect the committed changes.
• benschac added a commit: rCOMM42d15674e084: [web] [remove] [ENG-1022] add long arrow and implement in sidebar.

Revision Contents
Changeset List

PathSize
keyserver/
images/
3 lines
7 lines
web/
chat/
24 lines
19 lines

Diff 12588

View Options

keyserver/images/arrow.svg

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net
View Options

keyserver/images/long_arrow.svg

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net
View Options

web/chat/chat-thread-list.css

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net
View Options

web/chat/sidebar-item.react.js

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net