Add organizer logo to event pages #3530

Arashfa0301 · 2023-02-07T13:19:19Z

Description

Added organiser logo to event pages for user eye satisfaction and increased organiser pride. I am open to improvement suggestions. Important to note that the email link is removed since I believe it has minimal importance being here and it could be easily found at group page.

Result

before

after

Testing

I have thoroughly tested my changes on mobile and window views

ivarnakken

I'm not sure if I like the addition of the author's profile picture, since I don't think it is really needed, and I don't like "their face" becoming the face of the event, if that makes any sense ..

I think making group logos more visible is a nice touch though! 💯

app/routes/events/components/EventDetail/index.tsx

danielyanghansen · 2023-02-07T13:28:11Z

Design-wise, I like the concept, and it's nice to have a visual connection to the organizer.
However, I feel it's important to keep the email address there somehow, as this is important for the participants to know who to contact.
Maybe show the email on hover?

ivarnakken · 2023-02-07T14:22:47Z

However, I feel it's important to keep the email address there somehow, as this is important for the participants to know who to contact.
Maybe show the email on hover?

I vouch for adding it on hover. You can find the email by clicking on the link and going to the group page. I think people will manage fine.

ivarnakken · 2023-02-08T23:16:19Z

@Arashfa0301
Remember to rebase from master instead of merging master into your working branch.

erlingfn

I like the addition, but I think it has to be implemented in a generic and extendible way if it should be done

app/routes/events/components/EventDetail/index.tsx

Arashfa0301 · 2023-02-15T17:35:00Z

I'm not sure if I like the addition of the author's profile picture, since I don't think it is really needed, and I don't like "their face" becoming the face of the event, if that makes any sense ..

I think making group logos more visible is a nice touch though! 💯

I have now removed it.

ivarnakken · 2023-02-15T22:48:09Z

app/routes/events/components/EventDetail/index.tsx

+          <Link to={groupLink} className={styles.organizerLogo}>
+            {groupLink && (
+              <Tooltip
+                key={event.responsibleGroup.id}
+                content={event.responsibleGroup.name}
+              >
+                <CircularPicture
+                  alt={event.responsibleGroup.name}
+                  src={event.responsibleGroup.logo}
+                  size={40}
+                />
+              </Tooltip>
            )}{' '}
-            {event.responsibleGroup.contactEmail && (
-              <a href={`mailto:${event.responsibleGroup.contactEmail}`}>
-                {event.responsibleGroup.contactEmail}
-              </a>
-            )}
-          </span>
+            {event.responsibleGroup.name}
+          </Link>


hmm is the tooltip really needed now that the name is next to the image? I suggest either removing it or putting the email address inside.

I shall remove it. The email looks very weird inside it 👍

Bumps [@sentry/integrations](https://github.com/getsentry/sentry-javascript) from 7.34.0 to 7.36.0. - [Release notes](https://github.com/getsentry/sentry-javascript/releases) - [Changelog](https://github.com/getsentry/sentry-javascript/blob/develop/CHANGELOG.md) - [Commits](getsentry/sentry-javascript@7.34.0...7.36.0) --- updated-dependencies: - dependency-name: "@sentry/integrations" dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

They had different paddings and margins, causing the "Påmeldinger" header to be pushed further up than the other headers. Resolves ABA-248

- Icons in the table header did not match the text color - Give delete icon a confirm modal for safety purposes - Delete icon was not centered

eikhr

Looks good! I like the change. You need to rebase this though, and there are a few changes that seem unrelated to the PR though, perhaps left in accidentally from a merge conflict?

eikhr · 2023-07-11T19:27:53Z

app/routes/events/components/EventDetail/index.tsx

@@ -315,26 +317,24 @@ export default class EventDetail extends Component<Props, State> {
          }
        : null,
    ];
-
+    c;


eikhr · 2023-07-11T19:30:08Z

package.json

@@ -34,7 +34,7 @@
    "@loadable/component": "^5.15.3",
    "@reduxjs/toolkit": "^1.9.2",
    "@sentry/browser": "^7.36.0",
-    "@sentry/integrations": "^7.34.0",
+    "@sentry/integrations": "^7.36.0",


Is this relevant to this PR?

eikhr · 2023-07-11T19:31:14Z

app/routes/users/components/UserSettingsOAuth2.tsx

+        <ConfirmModalWithParent
+          title="Bekreft handling"
+          message={`Er du sikker på at du vil fjerne token?`}
+          onConfirm={() => props.deleteOAuth2Grant(grant.id)}
+          closeOnCancel
        >
-          <Icon name="trash-outline" size={20} className={styles.deleteIcon} />
-        </Tooltip>
+          <Tooltip content="Fjern" style={{ marginTop: '-7px' }}>
+            <Icon
+              name="trash-outline"
+              size={18}
+              className={styles.deleteIcon}
+            />
+          </Tooltip>
+        </ConfirmModalWithParent>


This seems like a nice change, but it's not really relevant to the PR? At least mention it in the description

Arashfa0301 requested review from erlingfn, eikhr, LudvigHz, juniwbjerde and ivarnakken February 7, 2023 13:19

Arashfa0301 added discussion Pull requests that needs discussions, e.g. regarding controversial/big changes review-needed Pull requests that need review small-fix Pull requests that fix something small labels Feb 7, 2023

ivarnakken requested changes Feb 7, 2023

View reviewed changes

app/routes/events/components/EventDetail/index.tsx Outdated Show resolved Hide resolved

app/routes/events/components/EventDetail/index.tsx Outdated Show resolved Hide resolved

Arashfa0301 force-pushed the add-organizer-logo branch from 329330a to a105fd8 Compare February 7, 2023 17:32

erlingfn requested changes Feb 9, 2023

View reviewed changes

app/routes/events/components/EventDetail/index.tsx Outdated Show resolved Hide resolved

app/routes/events/components/EventDetail/index.tsx Outdated Show resolved Hide resolved

Arashfa0301 force-pushed the add-organizer-logo branch 2 times, most recently from 60e1dc1 to 08605f6 Compare February 15, 2023 17:32

Arashfa0301 requested a review from ivarnakken February 15, 2023 17:36

ivarnakken reviewed Feb 15, 2023

View reviewed changes

Arashfa0301 force-pushed the add-organizer-logo branch from 08605f6 to a105fd8 Compare February 18, 2023 13:40

dependabot bot and others added 3 commits February 18, 2023 14:41

Align overview headers correctly

48d92c1

They had different paddings and margins, causing the "Påmeldinger" header to be pushed further up than the other headers. Resolves ABA-248

Small table iteration

2dce1f7

- Icons in the table header did not match the text color - Give delete icon a confirm modal for safety purposes - Delete icon was not centered

Arashfa0301 force-pushed the add-organizer-logo branch from a105fd8 to 7cc0b2e Compare February 18, 2023 21:46

Arashfa0301 self-assigned this Feb 18, 2023

ivarnakken added the do-not-merge/WIP Pull requests that are "work in progress", and should not be merged label Feb 19, 2023

Arashfa0301 force-pushed the add-organizer-logo branch from 4a11693 to 38ca4f2 Compare February 21, 2023 10:38

Add organizer logo to event pages

9fa98ed

Arashfa0301 force-pushed the add-organizer-logo branch from 38ca4f2 to 9fa98ed Compare February 21, 2023 13:55

eikhr requested changes Jul 11, 2023

View reviewed changes

LudvigHz removed their request for review November 27, 2024 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add organizer logo to event pages #3530

Add organizer logo to event pages #3530

Arashfa0301 commented Feb 7, 2023 •

edited

Loading

ivarnakken left a comment

danielyanghansen commented Feb 7, 2023

ivarnakken commented Feb 7, 2023

ivarnakken commented Feb 8, 2023

erlingfn left a comment

Arashfa0301 commented Feb 15, 2023

ivarnakken Feb 15, 2023

Arashfa0301 Feb 16, 2023

eikhr left a comment

eikhr Jul 11, 2023

eikhr Jul 11, 2023

eikhr Jul 11, 2023

                         }
                       : null,
                   ];
+                  c;

Add organizer logo to event pages #3530

Are you sure you want to change the base?

Add organizer logo to event pages #3530

Conversation

Arashfa0301 commented Feb 7, 2023 • edited Loading

Description

Result

Testing

ivarnakken left a comment

Choose a reason for hiding this comment

danielyanghansen commented Feb 7, 2023

ivarnakken commented Feb 7, 2023

ivarnakken commented Feb 8, 2023

erlingfn left a comment

Choose a reason for hiding this comment

Arashfa0301 commented Feb 15, 2023

ivarnakken Feb 15, 2023

Choose a reason for hiding this comment

Arashfa0301 Feb 16, 2023

Choose a reason for hiding this comment

eikhr left a comment

Choose a reason for hiding this comment

eikhr Jul 11, 2023

Choose a reason for hiding this comment

eikhr Jul 11, 2023

Choose a reason for hiding this comment

eikhr Jul 11, 2023

Choose a reason for hiding this comment

Arashfa0301 commented Feb 7, 2023 •

edited

Loading