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

Update flipper and resolve breaking change #17159

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Conversation

rmtolmach
Copy link
Contributor

Summary

  • This work is behind a feature toggle (flipper): NO
  • This PR updates the flipper gem (and its dependencies) and resolves the breaking change from the update. In the flipper PR UI: Replace breadcrumbs with configurable nav flippercloud/flipper#853, breadcrumbs were removed, which made the UI break and tests fail (see Bump flipper-ui, flipper, flipper-active_record and flipper-active_support_cache_store #16396 (comment) for details). Shoutout to @ryan-mcneil for writing tests that caught the error 🤜 🤛
  • I added a link to our docs. If people don't like this, I'm happy to remove it.
  • We lose the Home button, but it sent you to the same page as the Features button, so we're not loosing anything useful.
  • One of the nav items flipper was showing by default with the upgrade was Settings. I personally don't think this is worth the nav space, so I've removed it from the navigation. It still exists (https://staging-api.va.gov/flipper/settings), but there's no link to it anywhere. But if this is something we think people will find useful, I can add it back in.
  • To reproduce this bug on master, run bundle update flipper, start your rails console and then navigate to http://127.0.0.1:3000/flipper/features. And/or run tests in spec/requests/flipper_request_spec.rb after updating flipper.
  • The solution is to incorporate the changes made to lib/flipper/ui/views/layout.erb into our file of the same name.
  • I'm on the Platform Product team. The Backend CoP is responsible for updating gems.

Related issue(s)

Testing done

  • New code is covered by unit tests - flipper have their own tests.
  • Describe what the old behavior was prior to the change - see screenshots and summary sections.
  • Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing - I clicked around in the UI locally to make sure the links worked.

Screenshots

Before (w/ breadcrumbs):
image

After (without breadcrumbs):
image

What areas of the site does it impact?

The Flipper UI in each env (https://staging-api.va.gov/flipper, for example)

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

<nav>
<ul class="nav nav-underline mb-3 align-items-center">
<% Flipper::UI.configuration.nav_items.each do |item| %>
<ul class="vads-u-margin--0 vads-u-padding--0 nav-item">
Copy link
Member

Choose a reason for hiding this comment

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

We might want to update the ul class according to this PR.
Here's the layout file:https://github.com/flippercloud/flipper/blob/966a956f36cd0ef6a838b9ae6b20818c5509766c/lib/flipper/ui/views/layout.erb. Here's what flipper suggests:

      <nav>
        <ul class="nav nav-underline mb-3 align-items-center">
          <% if Flipper::UI.configuration.application_href %>
            <li class="nav-item">
              <a class="nav-link" href="<%= Flipper::UI.configuration.application_href %>">
                ‹ App
              </a>
            </li>
          <% end %>


          <% Flipper::UI.configuration.nav_items.each do |item| %>
            <li class="nav-item">
              <a href="<%= url_for(item[:href]) %>" class="nav-link<%= " active" if request.url.start_with?(url_for(item[:href])) %>">
                <%= item[:title] %>
              </a>
            </li>
          <% end %>

          <li class="nav-item ms-auto text-muted small">
            <a href="https://www.flippercloud.io/docs/ui?utm_source=oss&utm_medium=ui&utm_campaign=docs">Docs</a>
            &bull;
            Version: <%= Flipper::VERSION %>
          </li>
        </ul>
      </nav>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I started with, but I changed it up just a little because I didn't love the way it looked 😆

Notable changes:

  • I removed the link to Settings
  • I made it an unordered list
  • I changed the Docs link to link to our internal docs
  • I removed the Version

Copy link
Contributor

@ryan-mcneil ryan-mcneil left a comment

Choose a reason for hiding this comment

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

yaaaas

@rmtolmach rmtolmach merged commit 331c0b3 into master Jun 18, 2024
20 checks passed
@rmtolmach rmtolmach deleted the flipper_update_rt branch June 18, 2024 16:29
@rmtolmach rmtolmach mentioned this pull request Jun 18, 2024
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