Skip to content

Conversation

@jeffesquivels
Copy link
Member

@jeffesquivels jeffesquivels commented Jan 22, 2025

More spin off commits from the sinpe branch.

The ones here are based on da0d25b, 8d9379a, 7a109ee and 663f583 without the sinpe movil specific parts.

@jeffesquivels jeffesquivels self-assigned this Jan 22, 2025
@sisou
Copy link
Member

sisou commented Jan 23, 2025

@onmax Can you explain why the RouteName enum is useful? It doesn't improve the situation in this PR I feel.

@onmax
Copy link
Member

onmax commented Jan 23, 2025

I introduced this because I often misspelled route names in the past. With TS, improve DX in both dev and building. However, I can undo it if you don't think it's worth it.

In code looks like:

router.push({ name: 'my-modal' }); // before
router.push({ name: RouteName.MyModal }); // after

plus, this change does not break anything

PS: Similar principle that you have commented before in one of my other PR

@sisou
Copy link
Member

sisou commented Jan 24, 2025

Well, for now you just moved the string definition from inside the route object to outside. But are the route names also going to be replaced in other places? Only then it makes sense.

@onmax
Copy link
Member

onmax commented Jan 24, 2025

See #255

@jeffesquivels
Copy link
Member Author

jeffesquivels commented Jan 24, 2025

Ah sorry, that's probably a mistake on my part, I didn't realized there was a commit where everything else got changed (I thought this was just going to be a partial migration kind of thing and then route names in other places would get changed whenever that code got changed in the future).

onmax and others added 5 commits January 27, 2025 17:00
@jeffesquivels jeffesquivels force-pushed the jeff/misc_from_sinpe_branch branch from 91fffb4 to d8f88d1 Compare January 27, 2025 23:01
@sisou sisou merged commit d8f88d1 into master Jan 28, 2025
1 check passed
@sisou sisou deleted the jeff/misc_from_sinpe_branch branch January 28, 2025 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants