Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add destination settings #475

Merged
merged 30 commits into from
Mar 10, 2023
Merged

Add destination settings #475

merged 30 commits into from
Mar 10, 2023

Conversation

podliashanyk
Copy link
Contributor

@podliashanyk podliashanyk commented Jan 30, 2023

Changes made:

  • Settings are replaced with Destinations

Remaining tasks:

NB! This PR must be rebased onto #474 in order for implementation to work

@podliashanyk podliashanyk added enhancement New feature or request size: large APIv2 Compatibility with APIv2 is guaranteed, may or may not be compatible with other API versions labels Jan 30, 2023
@podliashanyk podliashanyk self-assigned this Jan 30, 2023
This was referenced Jan 30, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2023

Codecov Report

Merging #475 (476dd6c) into master (4a32cd6) will decrease coverage by 2.96%.
The diff coverage is 4.06%.

@@            Coverage Diff             @@
##           master     #475      +/-   ##
==========================================
- Coverage   56.17%   53.22%   -2.96%     
==========================================
  Files          83       85       +2     
  Lines        3311     3506     +195     
  Branches      695      723      +28     
==========================================
+ Hits         1860     1866       +6     
- Misses       1443     1632     +189     
  Partials        8        8              
Impacted Files Coverage Δ
src/App.tsx 83.33% <ø> (ø)
src/components/header/Header.tsx 71.01% <ø> (ø)
src/components/destinations/NewDestination.tsx 2.47% <2.47%> (ø)
src/components/destinations/Destination.tsx 2.85% <2.85%> (ø)
src/components/destinations/DestinationsList.tsx 4.54% <4.54%> (ø)
src/components/destinations/DestinationGroup.tsx 5.88% <5.88%> (ø)
src/views/destinations/DestinationsView.tsx 33.33% <33.33%> (ø)
src/colorscheme.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@podliashanyk podliashanyk added the rr PLEASE review this PR! Remove after PR is closed/merged label Jan 30, 2023
Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

This looks really good! And I got it to work with the MS Teams plugin.

I only have two little comments (not sure if they belong here or if I should make a new issue for that):

  1. When changing a destination (I added a label) it shows the message "Created new destination: Email: [email protected]" instead of indicating that I changed a destination. This might be confusing.
  2. When adding a new destination with a label it might be nice to have the success message "Create new destination: label of the destination" and only using the suggested_label if no label is provided by the user.

@stveit
Copy link
Contributor

stveit commented Jan 31, 2023

Review from esteemed critic:

  • for existing destinations, when clicking on upper part, above the line where you write (where it says, Title, phone number for instance), it just selects the destination as a whole, so if you click a bit above the Title input field, the input field is not selected and you can not start typing. so you have to click on the lower end of it. The flash-effect triggers the same no matter where you click, so it kinda feels like it worked if you clicked too high up but its not actually properly selected
  • pressing enter does not save when editing existing destination
  • ditto as above for creating a new one, enter does not create
  • Destination title at the top and the + sign for creating a new one are not lined up properly
  • If you try to create a new email destination and press create, if the email is invalid it will wipe both the title and email input box. ideally it should just give the error and not delete anything.
    if you only made a typo in the email, you would rather fix it instead of writing the whole thing again. and deleting title because email is wrong does not make sense. (same for sms, presumably same for all destination types)
  • no spacing between create and discard buttons
  • the flashing effect when clicking anywhere on an existing destination feels like a lot rn
  • if you edit a destination (changing value for emailphone number) the save button is inactive before you change anything. if you make a change, but then revert the change by either ctrl-z or just manually changing it back you can now press the save button
  • if you have existing destinations and open the destination view, it looks super weird while the stuff is loading
  • changing a destination gives the message "created destination"
  • under Creating new dsetination, you can click the discard button even though you havent written anything. But for existing destinations, you cant click save without changing something. inconsistent behaviour.
  • if you make an invalid change to a phone number and click save, you will get error and save button will become inactive. The button becoming inactive makes it feel like the change worked, even though it clearly didnt.
  • If you edit an existing phone number, you can add up to 2 letters to the number and when you try to save you will get the error message that the number already exists, instead of it being invalid. probably a backend thing? or does a few letters just not matter for real phone numbers? seems weird

@stveit
Copy link
Contributor

stveit commented Jan 31, 2023

If you edit an existing phone number, you can add up to 2 letters to the number and when you try to save you will get the error message that the number already exists, instead of it being invalid. probably a backend thing? or does a few letters just not matter for real phone numbers? seems weird

the same for creating basically, it allows you to create a phone number with 2 letters. It seems like the phone number library allows up to 2 letters but just strips them, so when editing it says "same number" because after stripping it becomes the same number

@stveit
Copy link
Contributor

stveit commented Jan 31, 2023

When deleting existing destination it says " Discard Are you sure you want to discard changes?" should probably be text more fitted for deleting stuff

@stveit
Copy link
Contributor

stveit commented Mar 6, 2023

  • The fixes have made it so that there is no visual feedback when you click on something that doesnt do anything, this is good. But theres a different issue regarding what type of cursor is shown. This is prob a bad explanation, so ask me for demonstration if u need. For example, when you hover over the "SAVE" button, the cursor looks different, like you can click on what you hovering above. The same "clickable" cursor is shown if youre hovering in the white space of the 'Title' or 'Email address' box, but if you click there nothing happens. When hovering on the 'Title' or 'email address' labels, the cursor shows the "standard" cursor instead of the clickable one. I would expect the clickable cursor to be shown when hovering the title, since clicking the title will select the corresponding input box. And i would expect the standard cursor to show up when hovering over the white area where clicking has no effect
  • Destination title and + button still dont look lined up to me
  • When both Email and sms boxes are extended, there is no spacing between them

Tested using Firefox

@stveit
Copy link
Contributor

stveit commented Mar 6, 2023

image
Also got a weird bug where the label collapses into the input box even though something is written there, but im not able to replicate it

@podliashanyk
Copy link
Contributor Author

  • The fixes have made it so that there is no visual feedback when you click on something that doesnt do anything, this is good. But theres a different issue regarding what type of cursor is shown. This is prob a bad explanation, so ask me for demonstration if u need. For example, when you hover over the "SAVE" button, the cursor looks different, like you can click on what you hovering above. The same "clickable" cursor is shown if youre hovering in the white space of the 'Title' or 'Email address' box, but if you click there nothing happens. When hovering on the 'Title' or 'email address' labels, the cursor shows the "standard" cursor instead of the clickable one. I would expect the clickable cursor to be shown when hovering the title, since clicking the title will select the corresponding input box. And i would expect the standard cursor to show up when hovering over the white area where clicking has no effect

Fixed all mentioned cursors, except for labels. MUI Buttons labels (which are programmatically connected to input fields) have default cursors on-hover, not pointer cursor as you suggest. I presume they follow the CSS specification. I suggest we also follow this specification and not change the cursor for labels. Pointer cursor is best suited for links and buttons.

  • When both Email and sms boxes are extended, there is no spacing between them

Fixed

@podliashanyk
Copy link
Contributor Author

image
Also got a weird bug where the label collapses into the input box even though something is written there, but im not able to replicate it

Could not reproduce this either 🤷‍♀️

@podliashanyk
Copy link
Contributor Author

  • Destination title and + button still dont look lined up to me

Fixed

@stveit
Copy link
Contributor

stveit commented Mar 8, 2023

everything seems OK now 👍

@podliashanyk
Copy link
Contributor Author

This looks really good! And I got it to work with the MS Teams plugin.

I only have two little comments (not sure if they belong here or if I should make a new issue for that):

  1. When changing a destination (I added a label) it shows the message "Created new destination: Email: [email protected]" instead of indicating that I changed a destination. This might be confusing.
  2. When adding a new destination with a label it might be nice to have the success message "Create new destination: label of the destination" and only using the suggested_label if no label is provided by the user.

Fixed!

Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

Nearly all changes that @stveit and I requested are integrated now, the only old thing that is still open and doesn't have a separate issue is that when creating a destination there is no spacing between the "Create" and the "Discard" button.

And two more things that I noticed now:

  • it is fixed that when creating a destination with a label it says "Created new destination: label", but when updating or deleting it still says "Updated/Deleted destination: suggested_label"
  • when deleting a destination it says "Are you sure you want to delete destination?" - my suggestion would be "Are you sure you want to delete the destination?" or "Are you sure you want to delete this destination?"

@podliashanyk
Copy link
Contributor Author

Nearly all changes that @stveit and I requested are integrated now, the only old thing that is still open and doesn't have a separate issue is that when creating a destination there is no spacing between the "Create" and the "Discard" button.

And two more things that I noticed now:

  • it is fixed that when creating a destination with a label it says "Created new destination: label", but when updating or deleting it still says "Updated/Deleted destination: suggested_label"
  • when deleting a destination it says "Are you sure you want to delete destination?" - my suggestion would be "Are you sure you want to delete the destination?" or "Are you sure you want to delete this destination?"

Fixed

Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

All looks great! All changes are integrated and I'm very happy with how everything looks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIv2 Compatibility with APIv2 is guaranteed, may or may not be compatible with other API versions enhancement New feature or request rr PLEASE review this PR! Remove after PR is closed/merged size: large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants