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

refactor(web): improve access to "Install", "Download logs", and "Installer options" actions #1690

Merged
merged 15 commits into from
Oct 28, 2024

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Oct 22, 2024

Problem

The Install button is kind of hidden in the Overview page, where users have to come back after fixing installation issues (if any) for start the installation. Twice effort for users: to guess where such a button is and to come back to hit it once the installer is happy and ready to proceed.

Additionally, the Download logs and Installer options actions are mounted in the application sidebar, which is not always available, making impossible to access them in screens such as Product Selection or Installation Finished.

Solution

Move all these actions to the top bar, which is always mounted, visible, and accessible. To do so, many changes were needed here and there to achieve visible changes listed below. See the PR commit by commit to know more.

  • Move the “Install” button to the top bar
  • Drop the “Install” button from the Overview Page
  • Drop the cog icon from the top bar and use a PF/Drowdown instead
  • Move “Installer options" to the top bar dropdown
  • Move “Download logs” to the top bar dropdown
  • Keep the “Change product” in the sidebar until we find a better place for it, but with better look&feel.

Apart from the above and the needed changes to make it possible, the About component has been deleted too since it was kind of useless at this moment.

Notes

Screenshots

Screen Before After
Product selection before_product_selection download_logs_and_installer_option_from_product_selection
Overview with issues before_overview_with_issues overview_page_issues_link
Overview w/o issues before_overview_wo_issues overview_page_install_button
Not overview page with issues before_not_overview_with_issues not_overview_page_issues_link
Not overview page w/o issues before_not_overview_wo_issues not_overview_page_install_button
Installation progress before_installation_progress installer_options_not_available_during_installation
Installation finished before_installation_finished installation_finished

Because at this moment it does not provide any useful information for
the user. Moreover, most probably a user in front of the Agama UI is
aware of what Agama is, especially now that it has its own documentation
site.
imobachgs added a commit that referenced this pull request Oct 23, 2024
## Problem 

After changes introduced in
#1693, the `LogsButton`
component has become obsolete and does not work as expected.

## Solution

Use an old plain HTML link pointing to the expected URL and using the
[download](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#download)
attribute.

## Additional note

Please, bear in mind that this is kind of intermediate change to make
the link work again. It will be superseded (and probably improved) by
the WIP #1690

## Testing

- Tested manually
A step forward to simplify or at least improve how the application is
laid out with full header and sidebar or not depending on the route or
the action the installer is displaying or triggering.

There are a lot of changes here and there, but basically what this
commit does is

* Drop SimpleLayout component
* Rename Main to Layout and add two convenience predefined variants:
  Full and Plain.
* Adapt layout/Header for displaying more or less things depending on
  props, sent via Layout component.
* Remove the installer options "cog button" in favor of a dropdown in
  the header right corner and move the installer options as an element
  inside. Such a dropdown will hold the "Download logs" actions in the
  short term.
* Adapt needed parts to accommodate above changes.
Where it is hold by the "options menu/dropdown". That way, such an
action is now available always, no matter if the application sidebar has
been mounted or not.

Somehow, it moves forward the refactor started at
#1694.
<MenuToggle
ref={toggleRef}
onClick={toggle}
aria-label={_("Options toggle")}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆘 Help needed to look for a better label for this "application options menu/dropdown". Please, note that it is not "Installer options", but contains an entry for it. What is more, known "Installer options" are actually a "Installer Localization options", but both terms are kind of... "confusing?"

installation: "/installation",
installationProgress: "/installation/progress",
installationFinished: "/installation/finished",
logs: "/api/manager/logs.tar.gz",
Copy link
Contributor Author

@dgdavid dgdavid Oct 24, 2024

Choose a reason for hiding this comment

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

@imobachgs

I've added this path over here, but it smells a bit. If you have a better idea please let me know.

For more context, see the commit in which it was added, 4dfbc5b

within(menu).getByRole("menuitem", { name: "Installer Options" });
});

it.todo("allows downloading the logs");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

☝️ I've added this because at this moment I've no idea on how test that the download link is triggered as expected.

It implies to remove it, and its ad hoc section, from the Overview page.

As an intermediate step, it also change the presentation of the Overview
page, which only uses the "two columns" layout if there is any issue to
render. Not polished in details because it is subject to future changes
to make the issues list always accessible/visible from any other place.
Comment on lines +87 to +88
title={_("Installation blocking issues")}
description={_("Before installing, please check the following problems.")}
Copy link
Contributor Author

@dgdavid dgdavid Oct 25, 2024

Choose a reason for hiding this comment

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

🆘 Another place where I need help with texts. But please, do not invest too much time because if we reach an agreement such a section will dissapear from the Overview page soon. For more context, see the WIP idea: https://trello.com/c/Cm6AUD2Y (internal link).

They stop working by the lack of `useAllIssues` mock after moving the
core/InstallaButton to the core/Header and start using such and issues
query to decide if the button is displayed or not.
@@ -62,6 +62,7 @@ jest.mock("~/queries/l10n", () => ({
jest.mock("~/queries/issues", () => ({
...jest.requireActual("~/queries/issues"),
useIssuesChanges: () => jest.fn(),
useAllIssues: () => ({ isEmtpy: true }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ ⚠️ ⚠️

After moving the core/InstallButton to the top bar and using the useAllIssues to decide if it should be shown (aka mounted) or not, I have had to add this mock here and in the components/core/Header.test.tsx file to make these tests work again.

This made me ask myself if would be a good idea to have some test util for always have issues mocked and influence them when needed.

For adding some space between the action and the viewport edges, without
investing too much time on it because its final appeareance and location
is not decided yet.
dgdavid added a commit that referenced this pull request Oct 27, 2024
More specifically, the .js or .jsx files touched in previous commits for
#1690.

Apart from the expected changes to make the migration possible, there
were two changes that worth a remark:

  * Make optional `description` and `icon` for the software/Product
    type.
  * Add the `custom.d.ts` file and make tsconfig.json file aware of it
    in order to avoid TypeScript complaints in the src/layout/Icon.ts
    file because missing types in the icons library. To know more read
    https://webpack.js.org/guides/typescript/#importing-other-assets
More specifically, the .js or .jsx files touched in previous commits for
#1690.

Apart from the expected changes to make the migration possible, there
were two changes that worth a remark:

  * Make optional `description` and `icon` for the software/Product
    type.
  * Add the `custom.d.ts` file and make tsconfig.json file aware of it
    in order to avoid TypeScript complaints in the src/layout/Icon.ts
    file because missing types in the icons library. To know more read
    https://webpack.js.org/guides/typescript/#importing-other-assets
@dgdavid dgdavid force-pushed the ui-improvements-202410 branch from c89ee76 to a4fd3d2 Compare October 27, 2024 23:05
@dgdavid dgdavid force-pushed the ui-improvements-202410 branch from a4fd3d2 to 1c457ee Compare October 27, 2024 23:10
@dgdavid dgdavid changed the title refactor(web): UI improvements refactor(web): improve access to "Install, Download logs, and Installer options actions Oct 28, 2024
@dgdavid dgdavid changed the title refactor(web): improve access to "Install, Download logs, and Installer options actions refactor(web): improve access to "Install", "Download logs", and "Installer options" actions Oct 28, 2024
@dgdavid dgdavid marked this pull request as ready for review October 28, 2024 09:48
As a counterpart of the InstallButton, an IssuesLink is displayed when
the installation is not possible to make the user aware why the
InstallButton is not there.

This is a temporary navigation link to the overview page that, most
probably, will be replaced by a toggler for a Notification Drawer
https://www.patternfly.org/components/notification-drawer always
accessible from the top bar.
It make no sense to show neither, InstallButton nor IssuesLink, when the
installer is configuring a product. So, this change avoid mounting them
at product progress path.
@dgdavid
Copy link
Contributor Author

dgdavid commented Oct 28, 2024

Thanks for the approval @imobachgs

Since looks we do not have better alternatives for the things I commented above, let's go ahead and improve them when inspiration decides to come into scene 😉

@dgdavid dgdavid merged commit a099d97 into master Oct 28, 2024
5 checks passed
@dgdavid dgdavid deleted the ui-improvements-202410 branch October 28, 2024 20:19
dgdavid added a commit that referenced this pull request Oct 29, 2024
It was broken when migration layout/Icon.jsx to layout/Icon.tsx in
#1690
dgdavid added a commit that referenced this pull request Oct 29, 2024
## Problem

Unfortunately, #1690 broken a
type import at core/EmptyState component that has been cough [by
OBS](https://build.opensuse.org/package/live_build_log/systemsmanagement:Agama:Devel/agama-web-ui/openSUSE_Tumbleweed/x86_64)


## Solution

To use the right type
(d9554b1)
and take the opportunity for migrating the component to TypeScript
(9719d0b)
which makes possible to get rid of a `@ts-expect-error` directive
(749db9e)

## Testing

Tested manually by running `NODE_ENV=production npm run build`
dgdavid added a commit that referenced this pull request Nov 27, 2024
…on always visible (#1778)

## Problem

[A month ago](#1690) , the
'Install' button achieved more visibility after being moved to the
sticky header, allowing users to access it from any page/section/screen.

But that was just an intermediate step towards improving the 'Install'
action and the discoverability of 'Installation setup problems,' which
sadly introduced some drawbacks:

* The Install button is displayed dynamically, which still _forces_
users to figure out where it will appear when it’s not visible from the
start.
* When installation is not possible, users have to navigate to the
Overview page to see what is making their setup invalid.


## Solution

* Keep the install button always enabled and visible, triggering the
installation when the setup is valid or displaying found _issues_ when
not, and
* Allow users to check found problems always, not only  in the overview 

## A discarded alternative

At commit
6ecdceb,
this set of changes was in a bit different shape, where the interface
rendered a disabled "Install" button and an enabled "Warning" button to
display the setup problems. Additionally, it featured a _tooltip_ to
explain to users why the "Install" button was disabled and where to
click to view the reasons in detail.

However, this approach created unnecessary complexity and moving pieces,
not to mention the challenge of writing a clear, concise, yet complete
explanation for the _tooltip_ and keeping it up-to-date, including
translations

What's more, there is no reason to avoid keeping the "Install" button
**always active but behaving slightly different** depending on the setup
status:

* Displaying found problems when the installation setup is invalid
* Displaying the confirmation dialog when the setup is valid

This way, the user doesn't need to think. They can simply click the
"Install" button. If it's not possible, they can immediately start
fixing their setup. If it is possible, they just have to confirm they
want to begin the installation.

---

Related to not yet planned Trello card, https://trello.com/c/DmUOQN1Y
(internal link)
dgdavid added a commit that referenced this pull request Dec 10, 2024
Rendering questions across the app no longer works as expected after
#1690.

Commit f10153b changed how the content
outside the _installation settings context_ is handled, navigating to
its own path instead of rendering as part of `App` component.

_Mounting_ `Questions` component in `Layout` should restore the previous
behavior and fix #1820.
@imobachgs imobachgs mentioned this pull request Jan 10, 2025
imobachgs added a commit that referenced this pull request Jan 13, 2025
Update to release version 11.

* #1495
* #1564
* #1617
* #1618
* #1625
* #1626
* #1627
* #1628
* #1630
* #1631
* #1632
* #1633
* #1634
* #1635
* #1636
* #1639
* #1640
* #1641
* #1642
* #1643
* #1644
* #1645
* #1646
* #1647
* #1648
* #1649
* #1650
* #1651
* #1652
* #1654
* #1655
* #1656
* #1657
* #1660
* #1663
* #1666
* #1667
* #1668
* #1670
* #1671
* #1673
* #1674
* #1675
* #1676
* #1677
* #1681
* #1682
* #1683
* #1684
* #1687
* #1688
* #1689
* #1690
* #1691
* #1692
* #1693
* #1694
* #1695
* #1696
* #1698
* #1699
* #1702
* #1703
* #1704
* #1705
* #1707
* #1708
* #1709
* #1710
* #1711
* #1712
* #1713
* #1714
* #1715
* #1716
* #1717
* #1718
* #1720
* #1721
* #1722
* #1723
* #1727
* #1728
* #1729
* #1731
* #1732
* #1733
* #1734
* #1735
* #1736
* #1737
* #1740
* #1741
* #1743
* #1744
* #1745
* #1746
* #1751
* #1753
* #1754
* #1755
* #1757
* #1762
* #1763
* #1764
* #1765
* #1766
* #1767
* #1769
* #1771
* #1772
* #1773
* #1774
* #1777
* #1778
* #1785
* #1786
* #1787
* #1788
* #1789
* #1790
* #1791
* #1792
* #1793
* #1794
* #1795
* #1796
* #1797
* #1798
* #1799
* #1800
* #1802
* #1803
* #1804
* #1805
* #1807
* #1808
* #1809
* #1810
* #1811
* #1812
* #1814
* #1815
* #1821
* #1822
* #1823
* #1824
* #1825
* #1826
* #1827
* #1828
* #1830
* #1831
* #1832
* #1833
* #1834
* #1835
* #1836
* #1837
* #1838
* #1839
* #1840
* #1841
* #1842
* #1843
* #1844
* #1845
* #1847
* #1848
* #1849
* #1850
* #1851
* #1854
* #1855
* #1856
* #1857
* #1860
* #1861
* #1863
* #1864
* #1865
* #1866
* #1867
* #1871
* #1872
* #1873
* #1875
* #1876
* #1877
* #1878
* #1880
* #1881
* #1882
* #1883
* #1884
* #1885
* #1886
* #1888
* #1889
* #1890
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.

Agama "flashes" congratulations screen before installation begins
2 participants