Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

feat: add tags to events #9276

Merged
merged 7 commits into from
Oct 15, 2023
Merged

feat: add tags to events #9276

merged 7 commits into from
Oct 15, 2023

Conversation

Dun-sin
Copy link
Contributor

@Dun-sin Dun-sin commented Oct 3, 2023

Fixes Issue

Closes #5832

Changes proposed

  • add tags to events

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

image

Note to reviewers

@github-actions github-actions bot added issue linked Pull Request has issue linked models labels Oct 3, 2023
package-lock.json Outdated Show resolved Hide resolved
Copy link
Contributor

@amandamartin-dev amandamartin-dev left a comment

Choose a reason for hiding this comment

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

Hey dunsin! Looking good so far. I have a few notes and one oddity that may be due to my environment but will require another maintainer to test.

  1. In adding the tags, the UI form for adding an event has been altered. Please maintain the 1-column layout for now as no design changes should be part of this issue. If you would like to propose design changes here, we can handle that separately.
  2. The labels for are missing in the add event form
  3. When I was testing, although I can add an event and see the tags in the preview, on save I cannot see them live. This may be my environment as I had issues on another PR related to events seeing the updates (testing in gitpod) - needs confirmation from another maintainer
  4. attempted testing to see what happens when duplicate URL events with different tags are merged - unsuccessful due to the issue above.

Images for clarity - please let me know if you need any more detail. Thank you for working on this.
UI
no-tags

@Dun-sin
Copy link
Contributor Author

Dun-sin commented Oct 13, 2023

Hey dunsin! Looking good so far. I have a few notes and one oddity that may be due to my environment but will require another maintainer to test.

  1. In adding the tags, the UI form for adding an event has been altered. Please maintain the 1-column layout for now as no design changes should be part of this issue. If you would like to propose design changes here, we can handle that separately.
  2. The labels for are missing in the add event form
  3. When I was testing, although I can add an event and see the tags in the preview, on save I cannot see them live. This may be my environment as I had issues on another PR related to events seeing the updates (testing in gitpod) - needs confirmation from another maintainer
  4. attempted testing to see what happens when duplicate URL events with different tags are merged - unsuccessful due to the issue above.

Images for clarity - please let me know if you need any more detail. Thank you for working on this.
UI
no-tags

@amandamartin-dev for the review:

  1. I didn't change anything UI based in adding an event, except where to add the tags and I followed the instruction on where to put it from the issue itself, I'm kind of unclear as to where this UI change is really coming from, please clarify
  2. It's unclear what you mean you would have to clarify too.

@amandamartin-dev
Copy link
Contributor

  1. I didn't change anything UI based in adding an event, except where to add the tags and I followed the instruction on where to put it from the issue itself, I'm kind of unclear as to where this UI change is really coming from, please clarify

For point 1 - you have added a new div but if you take a look at the structure of the form, the CSS on the new div is different - this is likely the culprit. Take a look and see what you find and if you are stuck, I can help a little later.

  1. It's unclear what you mean you would have to clarify too.
    For this one, take a look at production and compare with your local version. You will notice that each form field has a label that is visible in production. This should be maintained in the update.

Production:
labels

PR version:
label-missing

@Dun-sin
Copy link
Contributor Author

Dun-sin commented Oct 13, 2023

  1. I didn't change anything UI based in adding an event, except where to add the tags and I followed the instruction on where to put it from the issue itself, I'm kind of unclear as to where this UI change is really coming from, please clarify

For point 1 - you have added a new div but if you take a look at the structure of the form, the CSS on the new div is different - this is likely the culprit. Take a look and see what you find and if you are stuck, I can help a little later.

  1. It's unclear what you mean you would have to clarify too.
    For this one, take a look at production and compare with your local version. You will notice that each form field has a label that is visible in production. This should be maintained in the update.

Production:
labels

PR version:
label-missing

Interesting because that's how it looked like when I did run it the first time without adding anything🤔 but it could also be from me I'm going to do two things: check the PR code for anything that I know for a fact I didn't add, the second thing would be to go to the code and uncomment the JSX change I did, if it makes a difference or it doesn't I will let you know.

@Dun-sin
Copy link
Contributor Author

Dun-sin commented Oct 14, 2023

I'm off the coding grid atm so couldn't do anything, thanks for the help @eddiejaoude

@eddiejaoude
Copy link
Member

No worries @Dun-sin

return null;
}

return <Tag name={trimmedTag} key={index} />;
Copy link
Member

Choose a reason for hiding this comment

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

The tag component is expecting an onClick event, so has a hover state

Screenshot 2023-10-15 at 04 32 02

Using the tag simple component I think would be better here, later if we add an on click event we can change it back

Screenshot 2023-10-15 at 04 37 27

Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Thank you 👍

I will address the remaining comments via a temporary branch

@eddiejaoude eddiejaoude dismissed amandamartin-dev’s stale review October 15, 2023 04:01

I will address via temporary branch

@eddiejaoude eddiejaoude changed the base branch from main to fix-pr-9276 October 15, 2023 04:02
@eddiejaoude eddiejaoude merged commit 1d048d1 into EddieHubCommunity:fix-pr-9276 Oct 15, 2023
13 checks passed
@eddiejaoude eddiejaoude mentioned this pull request Oct 15, 2023
6 tasks
eddiejaoude added a commit that referenced this pull request Oct 15, 2023
* feat: tags to events (#9276)

* feat: add tags while creating a new event and send it to the api

* feat: add tags to events

* chore: remove package-lock.json

* Update pages/account/manage/event/[[...data]].js

Co-authored-by: Eddie Jaoude <[email protected]>

---------

Co-authored-by: Eddie Jaoude <[email protected]>

* fix: tags to events

---------

Co-authored-by: Dunsin <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue linked Pull Request has issue linked models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] Events to have tags
3 participants