Custom url navigations #1269

vanillaHafer · 2023-07-24T20:20:42Z

Context

Currently, Spina::NavigationItems can only link to other main Spina pages and they cannot be edited. It would be nice if Custom URLs were able to be added in, to allow more customization to the user.

Changes proposed in this pull request

The 2 main changes in this PR are:

Being able to edit existing navigation items

2. Being able to use custom URLs with their labels, instead of having to select from the list of pages

Guidance to review

Everything existing functions as normal. All changes are simply additions.

Bramjetten

Thanks for working on this! I like the idea here, but I'm not sure about the UI implementation yet. Few things to consider:

How do you handle translations? This goes for the label, but also the URL
Instead of having a button "Add menu item", perhaps it should be two buttons: "Add page" and "Add URL" which will then have their own forms.

Bramjetten · 2023-07-25T17:28:42Z

app/models/spina/navigation_item.rb

@@ -25,6 +25,14 @@ def menu_title
      end
    end

+    def materialized_path


This could be rewritten as:

def materialized_path page&.materialized_path || url end

or like this if you prefer something a bit more explicit:

def materialized_path return page.materialized_path if page url end

Great suggestion. I went ahead and made those corrections.

Bramjetten · 2023-07-25T17:32:57Z

app/models/spina/navigation_item.rb


-    delegate :menu_title, :materialized_path, :draft?, :homepage?, to: :page
+    delegate :draft?, :homepage?, to: :page


I think these also need their own method now, defaulting to false when there's no page. We might be able to get away with allow_nil: true here.

Good call! That was an oversight on my part. Went ahead and added the allow_nil: true to take care of this.

Bramjetten · 2023-07-25T17:35:14Z

test/dummy/db/schema.rb

    t.integer "navigation_id", null: false
    t.integer "position", default: 0, null: false
    t.string "ancestry"
    t.datetime "created_at", precision: nil
    t.datetime "updated_at", precision: nil
+    t.string "url"
+    t.string "url_label"


Why not menu_title like for pages? How do you handle translations?

We ended up making changes so there is a url_title attribute that get translated through mobility.

mattpolito · 2023-07-25T18:18:05Z

Instead of having a button "Add menu item", perhaps it should be two buttons: "Add page" and "Add URL" which will then have their own forms.

@Bramjetten I guess it depends on if we have that design language elsewhere in the site. Personally I would think that I'm going to this section to add a navigation item... so already the language of "Add menu item" differs but at least it's a singular thing. If we add choices here, I would think that someone would always choose "Add URL" as it makes the most sense for a nav item.

For confusion sake, I would suggest that there be a single link to open a form where you'd then choose the kind of thing you want to create. In the end you're always creating "A menu item".

* URL string field * URL Label string field

* Use the page menu title if it has a page, otherwise use the url_label

* url && url_label * url || page_id * page_id is now optional

* url_placeholder * url_label_placeholder

* Now uses toggleComponenet * Can now use custom urls

mattpolito · 2023-07-25T21:30:19Z

@Bramjetten Also we noticed that a test failure was introduced by a previous commit so you'll see the fix for that in here as well.

Bramjetten · 2023-07-26T07:23:03Z

@mattpolito I've noticed some flakiness with a certain Resource test, so I might have to fix that anyway.

mattpolito · 2023-07-26T18:11:54Z

@Bramjetten do you have any additional feedback on this one?

Bramjetten · 2023-07-27T07:17:25Z

I would very much like to have this feature, so let's work on getting it in Spina!

Couple of things I notice in the UI in no particular order:

The "Choose page" select is a simple dropdown which is hard to use when you have many pages. Maybe we can borrow PageLink's select with search functionality.
Using a toggle to switch forms feels odd. Also, the window jumps when you do that.
The input fields don't have autofocus
Perhaps instead of a toggle, it can simply be a link to a different form
There's no validation on adding a URL
It doesn't support translations
If you can change the label for custom URLs, shouldn't you be able to do that for pages as well?
I feel the navigation items themselves should have a little indicator to tell you it's a custom URL and not a page

Couple of things I notice in the code:

Why not use/update the SwitchComponent?
Can we have one migration instead of two?
Instead of altering navigation_item_params in the controller, I'd prefer to use Turbo Frames to change the form instead of showing/hiding input fields with JS.

mattpolito · 2023-07-27T14:20:30Z

The "Choose page" select is a simple dropdown which is hard to use when you have many pages. Maybe we can borrow PageLink's select with search functionality.

I completely agree with this needing improvement. However it is not part of adding a custom url so I'd leave it for another PR.

Using a toggle to switch forms feels odd. Also, the window jumps when you do that.

Clearly you haven't been a fan of the toggle and I don't think a separate button is the right choice either. How do you feel about that modal opening with tabs? One for "Page" and another for "URL"?

The input fields don't have autofocus

This can be easily addressed

Perhaps instead of a toggle, it can simply be a link to a different form

See above comment regarding toggle/form

There's no validation on adding a URL

Technically there is a validation. At least one that says it has to be populated if you select the custom url option. Are you referring to something more specific validating the URL itself? Also due to how modals are implemented currently, It does not allow for errors to come back from turbo. We've been wanting to address this anyway. Again, I think that'd be change but not part of this PR.

It doesn't support translations

We added translations the other day. Did you not see that change or are you referring to something else?

If you can change the label for custom URLs, shouldn't you be able to do that for pages as well?

We didn't make that change because currently you have the mechanism to change a page label on the page itself. Not sure if we want to immediately tackle this but we think it should actually live here in the navigation item. That way we get a uniform experience as well as the ability to have have pages in multiple navigations that can have a different name than the singular one defined on the page.

I feel the navigation items themselves should have a little indicator to tell you it's a custom URL and not a page

I agree with this.

Why not use/update the SwitchComponent?

If you don't like the toggle, then there's no reason to use the switch component. So we just need to make a decision on UX.

Can we have one migration instead of two?

We CAN however we're addressing two different tables so it seems appropriate to have two different migrations. With that said... Is there a reason we're using integer migration versions vs timestamp?

Instead of altering navigation_item_params in the controller, I'd prefer to use Turbo Frames to change the form instead of showing/hiding input fields with JS.

This again refers to the decision that needs to be made about the UX.

Overall... we can get the changes in that we all agree on and let's discuss how you feel about the form UX so we can move forward with the other changes.

vanillaHafer · 2023-07-31T15:49:28Z

I went ahead and implemented those UI changes, so hopefully they are looking more like what you were expecting.

Removed tabs in favor of links

Changed indicator

Translations

As far as translations go, we're handling translations for the UI elements the same as all the other elements, by adding to the .yml files under config/locales/. And then regarding actual URL Title translations we are using a translation table for navigation items. As far as we understand, that's enough to handle translations, but please let us know if we are missing something.

Bramjetten · 2023-07-31T18:12:03Z

This is great!

What I mean with the translations is that I don't think the UI currently supports editing multiple translations. How can end-users add multiple translations for a menu item?

mattpolito · 2023-07-31T19:20:42Z

How can end-users add multiple translations for a menu item?

How is that working anywhere else? I don't recall seeing that functionality.

Bramjetten · 2023-07-31T19:23:29Z

How is that working anywhere else? I don't recall seeing that functionality.

vanillaHafer · 2023-07-31T22:12:46Z

Since we don't use translations for our application, the translation feature was a bit foreign to us. Thanks for being patient with us as we learn more about Spina to get this feature rolling.

Looking at the current implementation I was able to throw something together for the Navigations/Navigation Items.

As you can see on the Navigations screen we now have a Translation Component in the upper right:

Which will display Navigation Items using their correct translations:

When adding in a navigation, we can choose which translation we are adding:

After adding in a navigation item and giving it, say, an English translation first, we can now see that some translations are missing url_titles:

Which is also seen when editing that particular navigation item:

As usual, please give us any feedback you have on this so we can correct anything you think should change!

Bramjetten · 2023-08-13T11:22:41Z

I think we can improve the UI for switching locales. I think it’s best if we make it possible to fill in all translations in one go, as that’s probably the most common workflow. Perhaps for now it’s best to simply list the different locale fields from top to bottom and improve the UI further in a different PR. That way we can get this merged now and enable it for everyone :)

mattpolito · 2023-08-13T17:26:30Z

I think it’s best if we make it possible to fill in all translations in one go, as that’s probably the most common workflow.

@Bramjetten is there an example of this workflow currently in the codebase?

Bramjetten · 2023-08-13T21:21:15Z

No there isn’t, but the easiest way is to simply loop over all locales in Spina.config.locales and render the fields manually.

mattpolito · 2023-08-14T13:58:42Z

Since that is a new UI concept, I would really suggest it be separate from this PR. The data flow wouldn't change so doing it at a later time would not make a difference.

That request feels like a change that should be done across the entire UI.

Bramjetten · 2023-08-19T08:22:21Z

@mattpolito Agreed that we can merge this into the main branch now and clean up the UI in a separate PR. I’ll create an issue for it.

Bramjetten · 2023-08-19T08:24:40Z

@mattpolito Do you know why the tests are failing?

mattpolito · 2023-08-19T17:57:24Z

@Bramjetten thats an interesting failure. I'll look at it or have @vanillaHafer double check it. It's failing on Minitest so it may be a CI issue.

mattpolito · 2023-08-20T15:27:14Z

@Bramjetten suite on this branch was passing. The error that is currently happening seems to have been introduced from #1273

Allow ancestry to be set in custom pages

1175fc7

Bramjetten reviewed Jul 25, 2023

View reviewed changes

vanillaHafer added 9 commits July 25, 2023 16:25

Allow navigation_items to be edited

aa8d8b7

Added a toggle component

4bc3e05

Add Custom URLs to Spina::NavigationItem

5a059d1

* URL string field * URL Label string field

Add button toggle stimulus controller

6021e97

Extract menu_title from page_delegation

057813f

* Use the page menu title if it has a page, otherwise use the url_label

Validations for Spina::NavigationItem

8d9910b

* url && url_label * url || page_id * page_id is now optional

Add text locales for en.yml

9132a38

* url_placeholder * url_label_placeholder

Updated the navigation_item form

1e01b84

* Now uses toggleComponenet * Can now use custom urls

Extract materialized_path from page_delegation

c49f34e

vanillaHafer force-pushed the custom-url-navigations branch 4 times, most recently from 681a0c2 to 83747db Compare July 25, 2023 21:02

vanillaHafer added 7 commits July 26, 2023 09:48

Created generic page and resource factory values

323fb83

Basic testing around Spina::NavigationItem

2bf94ff

allow_nil for delegated methods without a page

7d2e468

Refactor menu_title and materialized_path in NavigationItem

2b67b78

Update toggle_component from wrong controller name

7cc4598

Condense validation logic into a with_options

8e9b426

Switch from url_lable to url_title using a translations table

6cb5783

vanillaHafer force-pushed the custom-url-navigations branch from 45f091c to 6cb5783 Compare July 26, 2023 13:48

Allow placeholders to be optionally passed in to TextFieldComponent

f941045

vanillaHafer force-pushed the custom-url-navigations branch 3 times, most recently from 524a4bd to ace1394 Compare July 31, 2023 15:38

Remove toggle in favor of separate forms

246ec9d

vanillaHafer force-pushed the custom-url-navigations branch from ace1394 to 246ec9d Compare July 31, 2023 15:45

vanillaHafer force-pushed the custom-url-navigations branch from 879cd8e to 972627e Compare July 31, 2023 21:41

Added in translation customizations for Navigation Items

8f4cdb2

vanillaHafer force-pushed the custom-url-navigations branch from 972627e to 8f4cdb2 Compare July 31, 2023 22:12

Clear out url_title for each translation when item is of page kind

8e4effc

Bramjetten mentioned this pull request Aug 19, 2023

Clean up the UI introduced in #1269 #1288

Closed

Bramjetten mentioned this pull request Aug 21, 2023

Test suite fails to run due to uninitialized constant error #1290

Closed

Bramjetten added 4 commits August 21, 2023 20:30

Clean up UI and code

3d54eac

Update Tailwind build

5d119f5

Mobility not needed anymore

d54fbc0

Remove unnecessary code

8c764ca

Bramjetten approved these changes Aug 22, 2023

View reviewed changes

Bramjetten merged commit 919e649 into SpinaCMS:main Aug 22, 2023
0 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom url navigations #1269

Custom url navigations #1269

vanillaHafer commented Jul 24, 2023

Bramjetten left a comment

Bramjetten Jul 25, 2023

vanillaHafer Jul 25, 2023

Bramjetten Jul 25, 2023

vanillaHafer Jul 25, 2023

Bramjetten Jul 25, 2023

mattpolito Jul 25, 2023

mattpolito commented Jul 25, 2023

mattpolito commented Jul 25, 2023

Bramjetten commented Jul 26, 2023

mattpolito commented Jul 26, 2023

Bramjetten commented Jul 27, 2023

mattpolito commented Jul 27, 2023 •

edited

Loading

vanillaHafer commented Jul 31, 2023

Bramjetten commented Jul 31, 2023

mattpolito commented Jul 31, 2023

Bramjetten commented Jul 31, 2023

vanillaHafer commented Jul 31, 2023

Bramjetten commented Aug 13, 2023

mattpolito commented Aug 13, 2023

Bramjetten commented Aug 13, 2023

mattpolito commented Aug 14, 2023 •

edited

Loading

Bramjetten commented Aug 19, 2023

Bramjetten commented Aug 19, 2023

mattpolito commented Aug 19, 2023

mattpolito commented Aug 20, 2023


		delegate :menu_title, :materialized_path, :draft?, :homepage?, to: :page
		delegate :draft?, :homepage?, to: :page

Custom url navigations #1269

Custom url navigations #1269

Conversation

vanillaHafer commented Jul 24, 2023

Context

Changes proposed in this pull request

Guidance to review

Bramjetten left a comment

Choose a reason for hiding this comment

Bramjetten Jul 25, 2023

Choose a reason for hiding this comment

vanillaHafer Jul 25, 2023

Choose a reason for hiding this comment

Bramjetten Jul 25, 2023

Choose a reason for hiding this comment

vanillaHafer Jul 25, 2023

Choose a reason for hiding this comment

Bramjetten Jul 25, 2023

Choose a reason for hiding this comment

mattpolito Jul 25, 2023

Choose a reason for hiding this comment

mattpolito commented Jul 25, 2023

mattpolito commented Jul 25, 2023

Bramjetten commented Jul 26, 2023

mattpolito commented Jul 26, 2023

Bramjetten commented Jul 27, 2023

mattpolito commented Jul 27, 2023 • edited Loading

vanillaHafer commented Jul 31, 2023

Removed tabs in favor of links

Changed indicator

Translations

Bramjetten commented Jul 31, 2023

mattpolito commented Jul 31, 2023

Bramjetten commented Jul 31, 2023

vanillaHafer commented Jul 31, 2023

Bramjetten commented Aug 13, 2023

mattpolito commented Aug 13, 2023

Bramjetten commented Aug 13, 2023

mattpolito commented Aug 14, 2023 • edited Loading

Bramjetten commented Aug 19, 2023

Bramjetten commented Aug 19, 2023

mattpolito commented Aug 19, 2023

mattpolito commented Aug 20, 2023

mattpolito commented Jul 27, 2023 •

edited

Loading

mattpolito commented Aug 14, 2023 •

edited

Loading