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

🌸✨ Marketplace: UI to manage Order::NotificationMethod #1562

Merged
merged 4 commits into from
Jul 1, 2023

Conversation

zspencer
Copy link
Member

@zspencer zspencer commented Jun 15, 2023

This is basically a copy-paste from Marketplace::DeliveryArea, so it is ripe for testing via the delivery_area_request_spec.rb

TODO:

  • Find a better SVG for the :letter icon
  • Write request specs for
    • #new
    • #create
    • #edit
    • #update
    • #destroy
  • Write a Component spec
  • Write a system spec to add, update, and then destroy an OrderNotificationMethod

After

Screenshot 2023-06-28 at 4 46 36 PM Screenshot 2023-06-28 at 4 46 32 PM Screenshot 2023-06-28 at 4 46 27 PM Screenshot 2023-06-28 at 4 46 23 PM

@@ -6,6 +6,7 @@ class SvgComponent < ApplicationComponent
gear: :gear,
map: :map,
receipt_percent: :receipt_percent,
letter: :receipt_percent,
Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably be a better SVG than the receipt one...

Copy link
Contributor

Choose a reason for hiding this comment

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

the ICON_MAPPINGS was named letter so I replaced it with letter icon in 85514fd

letter icon

But, since the icon is applied to a notification menu, I felt a bell icon worked better, thus I added it in a3e90d9.
bell icon

Please choose which on you want or suggest other icon suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see now, it's notification via email. Hm maybe the open_letter icon works better than.

Maybe we should also change the Order Notifications text to a clearer Order Email Notifications 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it's notifications via email... for now; but if you check the Order Notifications and History we do have plans to include at the very least Square Point of Sale; and maybe even SMS!

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I love the bell!

parent :marketplace_order_notification_methods, order_notification_method.marketplace
link t("marketplace.order_notification_methods.edit.link_to", contact_location: order_notification_method.contact_location)
order_notification_method.marketplace.location(child: :order_notification_methods)
end
Copy link
Member Author

Choose a reason for hiding this comment

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

One day we will not have to define crumbs because we have made them so consistent that we can just infer wht the crumb should be...

ONE DAY....

def edit_button
super(title: t("marketplace.order_notification_methods.edit.link_to", contact_location: contact_location),
href: order_notification_method.location(:edit))
end
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice if #edit_button, #edit_button?, #destroy_button, and #destroy_button? could be part of a RecordComponent or something so we don't have to define them every time.

@zspencer zspencer marked this pull request as draft June 15, 2023 01:46
@zspencer zspencer changed the title Marketplace: UI to manage OrderNotificationMethod 🌸✨ Marketplace: UI to manage OrderNotificationMethod Jun 15, 2023
@zspencer zspencer added ✨ feature Reduces Client's Burden or Grants them Benefits 🌸 Polish Improves the UX! labels Jun 15, 2023
Base automatically changed from marketplace/improve-order-notification-ui to main June 15, 2023 02:32
@zspencer zspencer changed the title 🌸✨ Marketplace: UI to manage OrderNotificationMethod 🌸✨ Marketplace: UI to manage Order::NotificationMethod Jun 15, 2023
@zspencer
Copy link
Member Author

zspencer commented Jun 28, 2023

I'm going to split out the addition of the OrderNotification page from the creation of the OrderNotification model; since that is a small enough concrete step.

@zspencer zspencer changed the base branch from main to components/add-bell-to-svg-component June 28, 2023 22:54
end.tap do |order_notification_method|
authorize(order_notification_method)
end
end
Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like this could be replaced across almost all our controllers with decent_exposure

Copy link
Contributor

Choose a reason for hiding this comment

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

🆒 TIL

@zspencer zspencer force-pushed the components/add-bell-to-svg-component branch from 155996c to 36545a7 Compare June 28, 2023 23:53
@zspencer zspencer force-pushed the marketplace/order-notification-method-ui branch from df841cc to 46052e0 Compare June 28, 2023 23:55
@zspencer
Copy link
Member Author

In an ideal world; we would not need nearly this much ceremony to sprout the CRUD UI for a new model. We would need to define the Model, ModelController, ModelPolicy, ModelPolicy::Scope, ModelComponent and Model::FormComponent and the appropriate locales and routes; but breadcrumbs and most, if not all, of the views should be pretty standardized.

I'm not sure if it's prudent for me to do that now-right-now; but I am very tempted to take a refactor pass because basic CRUD should not require 300 lines of code :-p.

@zspencer zspencer force-pushed the components/add-bell-to-svg-component branch from 36545a7 to 1a9fe75 Compare June 29, 2023 00:11
@zspencer zspencer force-pushed the marketplace/order-notification-method-ui branch from 8568bce to 97ff810 Compare June 29, 2023 00:14
Base automatically changed from components/add-bell-to-svg-component to main June 29, 2023 00:33
@zspencer zspencer marked this pull request as ready for review June 29, 2023 00:33
@zspencer zspencer force-pushed the marketplace/order-notification-method-ui branch from 97ff810 to 621b907 Compare June 29, 2023 00:33
@zspencer zspencer requested review from a team and KellyAH June 29, 2023 00:40
@zspencer zspencer assigned zspencer and unassigned KellyAH Jun 29, 2023
@zspencer zspencer force-pushed the marketplace/order-notification-method-ui branch from 621b907 to 29f34bc Compare June 29, 2023 01:40
Copy link
Member

@anaulin anaulin 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!

- #1511

🧹 `Marketplace`: Move into `Order::NotificationMethod` (#1564)

The top-level `marketplace` namespace is getting pretty cluttered, and
since this relates pretty squarely to the `Order` domain, and there is
already an `Order::PlacedMailer` and `Order::ReceivedMailer` it seemed
like a reasonable place to put it.

🥗 Request specs for `Order::NotificationMethods`
zspencer and others added 2 commits June 30, 2023 20:41
Interestingly enough, to `button` method pointed out a problem with
putting the `Order::NotificationMethod`, in that the *routing* to the
`notification_methods` is not in the `order`; which was causing the
inferred translation lookup to go squiggly.

I took the hint and pulled it out of the `Order` namespace.
@zspencer zspencer force-pushed the marketplace/order-notification-method-ui branch from fc1429b to 0995f7c Compare July 1, 2023 04:14
@zspencer zspencer merged commit d1ba3f7 into main Jul 1, 2023
@zspencer zspencer deleted the marketplace/order-notification-method-ui branch July 1, 2023 04:20
def change
create_table :marketplace_order_notification_methods, id: :uuid do |t|
create_table :marketplace_notification_methods, id: :uuid do |t|
Copy link
Member Author

Choose a reason for hiding this comment

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

🤦 I did a silly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature Reduces Client's Burden or Grants them Benefits 🌸 Polish Improves the UX!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants