Changes to links in sidebar, fix tooltips #6959

javfg · 2022-05-12T13:01:49Z

This PR addresses some additions and fixes to the sidebar links:

Adds an isEos setting into config.json as discussed in Customization of UI elements #6702 (comment)
Adds direct link to the side bar (maybe related to Show path in right sidebar #6840)
Adds an eos path to the side bar, shown if isEos
Ellipsizes long links so the sidebar does not get a scrollbar. Along with Fix tooltips overflowing on long words owncloud-design-system#2137, these two should close Break long words #6849.
Normalized some sidebar table method names (some of them were titleSomething, all are now labelSomething)
Removed the colons and made the headers of the table nowrap. This makes the table looks better, as there is no need to have colons in headers; and they are not extremely long

Current looks:

update-docs · 2022-05-12T13:01:52Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

CLAassistant · 2022-05-12T15:23:55Z

All committers have signed the CLA.

pascalwengerter

Didn't dive too deeply yet but already looks pretty good from a quick glance. Added some polishing comments

pascalwengerter · 2022-05-12T16:18:47Z

CHANGELOG.md

@@ -53,6 +54,15 @@ Summary
 Details
 -------

+* Enhancement - Changes to links in sidebar, fix tooltips: [#6959](https://github.com/owncloud/web/pull/6959)


Please add this as an item to https://github.com/owncloud/web/tree/master/changelog/unreleased folder (similar to how it's done in oCIS/reva) and revert your changes to the CHANGELOG.md since that's an autogenerated file ;)

pascalwengerter · 2022-05-12T16:20:28Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

    file() {
      return this.displayedItem.value
    },

+    isEos() {
+      return !!this.configuration.options?.eos


Please add the eos config options to https://owncloud.dev/clients/web/getting-started/#options (markdown file in the docs/ directory in this repo) and ideally also with a default value in

web/packages/web-runtime/src/store/config.js

Line 40 in 72fba44

options: {

pascalwengerter · 2022-05-12T16:26:09Z

Also, we should think twice about naming before merging a new configuration option (because changing these later would mean a breaking change in terms of semver). Maybe runningOnEos or buildForEos are more applicable than isEos or just eos?

javfg · 2022-05-16T13:27:44Z

All suggestions added! I changed eos to runningOnEos, the first of the options you proposed.

pascalwengerter

Just tried and it mostly looks good, have some code hygiene/a11y points to fix but that should be a quick one :)

pascalwengerter · 2022-05-17T07:48:19Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+          <th scope="col" class="oc-pr-s" v-text="eosPathLabel" />
+          <td>
+            <div class="oc-flex oc-flex-middle oc-flex-between oc-width-1-1">
+              <div class="oc-flex oc-flex-middle">


The wrapping div around the <p> tag is not necessary

pascalwengerter · 2022-05-17T07:48:31Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+          <th scope="col" class="oc-pr-s" v-text="directLinkLabel" />
+          <td>
+            <div class="oc-flex oc-flex-middle oc-flex-between oc-width-1-1">
+              <div class="oc-flex oc-flex-middle">


The wrapping div around the <p> tag is not necessary

pascalwengerter · 2022-05-17T07:49:00Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+                />
+              </div>
+              <oc-button
+                oc-tooltip="Copy EOS path"


Please use a label prop and this.$gettext() here

pascalwengerter · 2022-05-17T07:49:04Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+              <oc-button
+                oc-tooltip="Copy EOS path"
+                appearance="raw"
+                aria-label="Copy EOS path"


Please use a label prop and this.$gettext() here

pascalwengerter · 2022-05-17T07:49:16Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+                oc-tooltip="Copy EOS path"
+                appearance="raw"
+                aria-label="Copy EOS path"
+                class="oc-files-private-link-copy-url"


The class looks wrong

pascalwengerter · 2022-05-17T07:49:42Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+              <oc-button
+                oc-tooltip="Copy direct link"
+                appearance="raw"
+                aria-label="Copy direct link"


Please use a label prop and this.$gettext() here

pascalwengerter · 2022-05-17T07:49:55Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+                oc-tooltip="Copy direct link"
+                appearance="raw"
+                aria-label="Copy direct link"
+                class="oc-files-private-link-copy-url"


The class looks wrong

pascalwengerter · 2022-05-17T07:50:15Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+                :variation="copiedDirect ? 'success' : 'passive'"
+                @click="copyDirectLinkToClipboard"
+              >
+                <span text="EOS Path" />


Please use a label prop and this.$gettext() here

Actually already exists as eosPathLabel, just needs to be used

I'm thinking I should remove these (this and line 101). They are not showing because text is not a valid attribute so they are effectively empty. If I make them have something inside, we get this:

I assume there is no reason to have text there, but correct me if I'm wrong (pinging @diocas).

You could use v-text and a computed property to show text (text itself won't work), but agreed that you could use an icon for "visual" usage and then both aria-label and tooltip on the wrapping button for contextual/screenreader help

pascalwengerter · 2022-05-17T07:50:46Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+                :variation="copiedEos ? 'success' : 'passive'"
+                @click="copyEosPathToClipboard"
+              >
+                <span text="EOS Path" />


Please use a label prop and this.$gettext() here

Same as #6959 (comment).

pascalwengerter · 2022-05-17T07:53:22Z

packages/web-app-files/src/components/SideBar/Details/FileDetails.vue

+      return !!this.configuration?.options?.runningOnEos
+    },
+
+    hasSharees() {


From here to line 279 are not necessary in this component (possibly rebase-gone-wrong?), could you double-check and delete if you agree?

Correct, this was included by mistake. I'll remove it.

Co-authored-by: elizavetaRa <svillyfly@mail.ru> Co-authored-by: Javier Ferrer <javier.ferrer@cern.ch>

javfg · 2022-05-17T11:23:51Z

I've applied all the suggestions (also removed some more stuff which didn't belong, like a getter used in the unneeded code you mentioned).

Also, rebased to latest master.

Let me know if I missed something!

sonarcloud · 2022-05-17T11:35:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

73.4% Coverage
0.0% Duplication

pascalwengerter · 2022-05-17T14:44:20Z

packages/web-app-files/tests/unit/components/SideBar/Details/FileDetails.spec.js

+        configuration: function () {
+          return {
+            options: {
+              runningOnEos: true


We should test both variants, I can merge your PR into an intermediate branch and add this myself

pascalwengerter

Good to go, I'll resolve a unit test improvement in my intermediate branch

* Changes to links in sidebar, fix tooltips (#6959) * Changes to links in sidebar, fix tooltips Co-authored-by: elizavetaRa <svillyfly@mail.ru> Co-authored-by: Javier Ferrer <javier.ferrer@cern.ch> * Update tests * Apply suggestions * Fix labels, remove unused code Co-authored-by: Diogo Castro <diogo.castro@cern.ch> Co-authored-by: elizavetaRa <svillyfly@mail.ru> * Update unit tests & reword changelog Co-authored-by: Javier Ferrer <javier.ferrer@cern.ch> Co-authored-by: Diogo Castro <diogo.castro@cern.ch> Co-authored-by: elizavetaRa <svillyfly@mail.ru>

Author: Pascal Wengerter <pwengerter@owncloud.com> Date: Wed May 18 10:13:58 2022 +0200 Port EOS details to details tab in files sidebar (#6997) * Changes to links in sidebar, fix tooltips (#6959) * Changes to links in sidebar, fix tooltips Co-authored-by: elizavetaRa <svillyfly@mail.ru> Co-authored-by: Javier Ferrer <javier.ferrer@cern.ch> * Update tests * Apply suggestions * Fix labels, remove unused code Co-authored-by: Diogo Castro <diogo.castro@cern.ch> Co-authored-by: elizavetaRa <svillyfly@mail.ru> * Update unit tests & reword changelog Co-authored-by: Javier Ferrer <javier.ferrer@cern.ch> Co-authored-by: Diogo Castro <diogo.castro@cern.ch> Co-authored-by: elizavetaRa <svillyfly@mail.ru>

javfg force-pushed the fix-sidebar-links-upstream branch 4 times, most recently from 5214156 to 0e0efea Compare May 12, 2022 15:23

pascalwengerter requested changes May 12, 2022

View reviewed changes

javfg force-pushed the fix-sidebar-links-upstream branch 4 times, most recently from a3d337e to 0ef9042 Compare May 16, 2022 12:52

javfg requested a review from pascalwengerter May 16, 2022 13:27

javfg marked this pull request as ready for review May 16, 2022 14:08

javfg requested a review from fschade as a code owner May 16, 2022 14:08

diocas added the Early-Adopter:CERN label May 17, 2022

pascalwengerter requested changes May 17, 2022

View reviewed changes

pascalwengerter mentioned this pull request May 17, 2022

Break long words #6849

Closed

diocas and others added 4 commits May 17, 2022 13:11

Changes to links in sidebar, fix tooltips

992a511

Co-authored-by: elizavetaRa <svillyfly@mail.ru> Co-authored-by: Javier Ferrer <javier.ferrer@cern.ch>

Update tests

131e677

Apply suggestions

94dc258

Fix labels, remove unused code

07f166f

javfg force-pushed the fix-sidebar-links-upstream branch from 0ef9042 to 07f166f Compare May 17, 2022 11:21

pascalwengerter reviewed May 17, 2022

View reviewed changes

pascalwengerter changed the base branch from master to port-eos-details-in-sidebar May 17, 2022 16:11

pascalwengerter approved these changes May 17, 2022

View reviewed changes

pascalwengerter merged commit be4a5e1 into owncloud:port-eos-details-in-sidebar May 17, 2022

pascalwengerter mentioned this pull request May 17, 2022

Port EOS details to details tab in files sidebar #6997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to links in sidebar, fix tooltips #6959

Changes to links in sidebar, fix tooltips #6959

javfg commented May 12, 2022

update-docs bot commented May 12, 2022

CLAassistant commented May 12, 2022 •

edited

Loading

pascalwengerter left a comment

pascalwengerter May 12, 2022

pascalwengerter May 12, 2022

pascalwengerter commented May 12, 2022

javfg commented May 16, 2022

pascalwengerter left a comment

pascalwengerter May 17, 2022

pascalwengerter May 17, 2022

pascalwengerter May 17, 2022

pascalwengerter May 17, 2022

pascalwengerter May 17, 2022

pascalwengerter May 17, 2022

pascalwengerter May 17, 2022

pascalwengerter May 17, 2022

pascalwengerter May 17, 2022

javfg May 17, 2022

pascalwengerter May 17, 2022

pascalwengerter May 17, 2022

javfg May 17, 2022

pascalwengerter May 17, 2022

javfg May 17, 2022

javfg commented May 17, 2022

sonarcloud bot commented May 17, 2022

pascalwengerter May 17, 2022

pascalwengerter left a comment

Changes to links in sidebar, fix tooltips #6959

Changes to links in sidebar, fix tooltips #6959

Conversation

javfg commented May 12, 2022

update-docs bot commented May 12, 2022

CLAassistant commented May 12, 2022 • edited Loading

pascalwengerter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascalwengerter commented May 12, 2022

javfg commented May 16, 2022

pascalwengerter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javfg commented May 17, 2022

sonarcloud bot commented May 17, 2022

Choose a reason for hiding this comment

pascalwengerter left a comment

Choose a reason for hiding this comment

CLAassistant commented May 12, 2022 •

edited

Loading