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

🧹 Section: Remove Navigation in favor of SectionNavigation Gizmo #1989

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

zspencer
Copy link
Member

@zspencer zspencer commented Dec 4, 2023

Now that we have a Gizmo for navigating between Sections, we don't want to keep it everywhere.

- #1155
- #1988

Now that we have a Gizmo for navigating between Sections, we don't want
to keep it everywhere.
@zspencer zspencer requested review from a team December 4, 2023 18:50
@zspencer zspencer marked this pull request as draft December 4, 2023 19:04
@zspencer
Copy link
Member Author

zspencer commented Dec 4, 2023

I've gotten rid of the feature tests that cover the presumption that Sections will always have a navigation element. I took the time to use yarn exec -- cucumber-js --dry-run --format=usage to find all the step definitions that are unused and purge them as well.

Now that we're using the gizmo, it doesn't make sense to have end-to-end
tests that presume every Section will include functioality to navigate
between Sections.

This also gets rid of all the unused step definitions.
@zspencer zspencer force-pushed the furniture/section-navigation/remove-defaults branch from 40cd2c1 to 8209187 Compare December 4, 2023 19:16
@zspencer zspencer marked this pull request as ready for review December 4, 2023 19:21
@zspencer zspencer force-pushed the furniture/section-navigation/remove-defaults branch from 33fe75b to 4c0b8ec Compare December 4, 2023 19:50
data: { access_level: room.access_level, slug: room.slug, model: "room", id: room.id },
classes: "group self-stretch hover:bg-orange-50"
) do %>
<%= link_to polymorphic_path(room.location), class: "no-underline" do %>
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'm not sure why, but link_to is including the full url; which I think is related to our over-loading of url_for.

I didn't want to get into it; but it's annoying.

@zspencer zspencer added the 🧹 refactor Includes non-behavioral changes label Dec 4, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why use a system spec vs a view spec to test rendered content?

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 doesn't have a ViewComponent yet and I was doing a lot of other cleanup so I didn't want to add that change too.

Also since I was deleting System specs it felt better to create new system specs. A ViewComponent spec would definitely be a better call when we get to more permutations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a RoR convention (informal | formal) that a "view spec" must be backed by a ViewComponent? Or this is a Convene worldview?

Copy link
Member

@anaulin anaulin Dec 5, 2023

Choose a reason for hiding this comment

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

I'm not aware of there being such a convention in neither RoR at large nor Convene specifically.

That said, I personally don't see the point of view specs, and would much rather do that kind of testing either in smaller component specs, or as part of larger integration/system tests (depending on the level of granularity desired). A view spec to me feels like "testing HTML", which smells funky.

YMMV YDY YOLO

Copy link
Member Author

@zspencer zspencer Dec 5, 2023

Choose a reason for hiding this comment

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

Yea, I tend to not write Rails view specs because once they get to the point of complexity that I would want one, I wrap it into a ViewComponent.

Or like Ana said, I test it in a request or system spec until it gets ungainly.

Copy link
Contributor

@rosschapman rosschapman left a comment

Choose a reason for hiding this comment

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

I have one question about the test but on scan the rest looks good. Nice cleanup 🧹

Copy link
Contributor

@rosschapman rosschapman left a comment

Choose a reason for hiding this comment

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

I have one question about the test but on scan the rest looks good. Nice cleanup 🧹

@zspencer zspencer merged commit 4a2af3b into main Dec 5, 2023
@zspencer zspencer deleted the furniture/section-navigation/remove-defaults branch December 5, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 refactor Includes non-behavioral changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants