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

EditDataSources: Add EditDataSourceActions to EditDataSourcePages #64487

Merged
merged 9 commits into from
Apr 13, 2023

Conversation

mikkancso
Copy link
Contributor

@mikkancso mikkancso commented Mar 9, 2023

What is this feature?

Adding action buttons in the header of EditDataSourcePage at both occurrences, namely under Connections and Admin.
Screencast from 2023-03-09 09-00-36.webm

UPDATE:
[Topnav OFF]
Do not show buttons in the header. Leave buttons as they are in the footer.

Screenshot from 2023-04-05 11-40-57
Screenshot from 2023-04-05 11-40-50

[Topnav ON]
Header:

  • build a dashboard
  • explore
  • delete

Screenshot from 2023-04-05 11-38-54

Footer:

  • explore
  • save & test

That also means we want to remove the Back button.

Screenshot from 2023-04-05 11-39-15

Which issue(s) does this PR fix?:

Fixes #58311

@mikkancso mikkancso requested a review from a team as a code owner March 9, 2023 08:26
@mikkancso mikkancso requested review from mckn and removed request for a team March 9, 2023 08:26
@mikkancso mikkancso added this to the 9.5.0 milestone Mar 9, 2023
@mikkancso mikkancso added no-backport Skip backport of PR add to changelog labels Mar 9, 2023
@mikkancso mikkancso changed the title EditDataSources: add EditDataSourceActions to EditDataSourcePages EditDataSources: Add EditDataSourceActions to EditDataSourcePages Mar 10, 2023
Copy link
Contributor

@mckn mckn left a comment

Choose a reason for hiding this comment

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

Code looks good! I have one question. If the topnav feature toggle is disabled I get the following apperance. Is it by design?

Screenshot 2023-03-15 at 11 20 33

@mikkancso
Copy link
Contributor Author

If topnav is turned off then we put action buttons below the title, and that's by design - in general. For this particular case, I don't know if this is what we want. @teddyba, could you please take a look at the picture above?

@teddyba
Copy link

teddyba commented Mar 21, 2023

In the case where topnav is turned off, I think we should have the buttons in one place or another, but not both. It doesn't matter to me where they live - I think neither placement is ideal (the lower placement is not a good place for CTAs, and the top placement without the topnav makes the header cluttered.)

I would maybe lean towards the former option - keeping the buttons at the bottom if topnav is turned off.

One thing to note about the header with topnav - we should definitely keep the CTA as "Save and test" NOT "Save changes." The ability to test a datasource connection is really important to our users.

@grafanabot grafanabot removed this from the 9.5.0 milestone Apr 4, 2023
@grafanabot
Copy link
Contributor

This pull request was removed from the 9.5.0 milestone because 9.5.0 is currently being released.

@mikkancso
Copy link
Contributor Author

mikkancso commented Apr 5, 2023

I'm sorry folks, that I left this PR hanging for so long, I had many other duties.

We talked about this with @teddyba, and we came to the following proposal:

[Topnav OFF]
Do not show buttons in the header. Leave buttons as they are in the footer.

[Topnav ON]
Header:

  • build a dashboard
  • explore
  • delete

Footer:

  • explore
  • save & test

That also means we want to remove the Back button. We hope users can use the new navigation to navigate back.
I'm going to implement this now.

@mikkancso mikkancso force-pushed the mikkancso/58311-update-data-source-edit-page-header branch from 9933bf3 to 657a8e6 Compare April 5, 2023 09:50
@mikkancso mikkancso added this to the 10.0.0 milestone Apr 5, 2023
@mikkancso mikkancso requested review from mckn and a team April 5, 2023 09:53
Copy link
Contributor

@leventebalogh leventebalogh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mikkancso mikkancso force-pushed the mikkancso/58311-update-data-source-edit-page-header branch from 54dae2e to 70c1fdc Compare April 13, 2023 06:06
@mikkancso
Copy link
Contributor Author

Rebased the branch to re-trigger drone

@mikkancso mikkancso merged commit 659024f into main Apr 13, 2023
5 checks passed
@mikkancso mikkancso deleted the mikkancso/58311-update-data-source-edit-page-header branch April 13, 2023 07:53
alexmobo pushed a commit that referenced this pull request Apr 14, 2023
…4487)

* add EditDataSourceActions to EditDataSourcePages

* fix tests

* EditDSPage: do not show buttons in header if topnav is off

* remove delete button from the header

* EditDSPage: hide buttons from footer when topnav is on

* update tests

* rename ActionProps to Props

* wrap setting of feature toggle in act in jest test

* fix jest test by using waitFor
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Header of Data source Edit connection page
6 participants