-
Notifications
You must be signed in to change notification settings - Fork 157
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
[full-ci] Refactor extension appBar design/concept #8447
Conversation
90d4d02
to
006ff24
Compare
SonarCloud Quality Gate failed. |
Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/36230/20/1 💥 The acceptance tests failed on retry. Please find the screenshots inside ...
webUIPreview-mediaPreview_feature-L106.pngwebUIPreview-mediaPreview_feature-L114.pngwebUIPreview-mediaPreview_feature-L122.pngwebUIPreview-mediaPreview_feature-L138.pngwebUIPreview-mediaPreview_feature-L139.pngwebUIPreview-mediaPreview_feature-L14.pngwebUIPreview-mediaPreview_feature-L140.pngwebUIPreview-mediaPreview_feature-L143.pngwebUIPreview-mediaPreview_feature-L15.pngwebUIPreview-mediaPreview_feature-L153.pngwebUIPreview-mediaPreview_feature-L16.pngwebUIPreview-mediaPreview_feature-L18.pngwebUIPreview-mediaPreview_feature-L25.pngwebUIPreview-mediaPreview_feature-L35.pngwebUIPreview-mediaPreview_feature-L56.pngwebUIPreview-mediaPreview_feature-L57.pngwebUIPreview-mediaPreview_feature-L60.pngwebUIPreview-mediaPreview_feature-L68.pngwebUIPreview-mediaPreview_feature-L77.pngwebUIPreview-mediaPreview_feature-L84.pngwebUIPreview-mediaPreview_feature-L91.pngwebUIPreview-mediaPreview_feature-L98.png |
006ff24
to
f507c1a
Compare
f507c1a
to
35889ac
Compare
35889ac
to
2bf3bbf
Compare
packages/web-app-external/tests/unit/__snapshots__/app.spec.ts.snap
Outdated
Show resolved
Hide resolved
Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/36230/50/1 💥 The acceptance tests failed on retry. Please find the screenshots inside ...
webUIPreview-mediaPreview_feature-L106.pngwebUIPreview-mediaPreview_feature-L114.pngwebUIPreview-mediaPreview_feature-L122.pngwebUIPreview-mediaPreview_feature-L138.pngwebUIPreview-mediaPreview_feature-L139.pngwebUIPreview-mediaPreview_feature-L14.pngwebUIPreview-mediaPreview_feature-L140.pngwebUIPreview-mediaPreview_feature-L143.pngwebUIPreview-mediaPreview_feature-L15.pngwebUIPreview-mediaPreview_feature-L153.pngwebUIPreview-mediaPreview_feature-L16.pngwebUIPreview-mediaPreview_feature-L18.pngwebUIPreview-mediaPreview_feature-L25.pngwebUIPreview-mediaPreview_feature-L35.pngwebUIPreview-mediaPreview_feature-L56.pngwebUIPreview-mediaPreview_feature-L57.pngwebUIPreview-mediaPreview_feature-L60.pngwebUIPreview-mediaPreview_feature-L68.pngwebUIPreview-mediaPreview_feature-L77.pngwebUIPreview-mediaPreview_feature-L84.pngwebUIPreview-mediaPreview_feature-L91.pngwebUIPreview-mediaPreview_feature-L98.png |
SonarCloud Quality Gate failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I really like the feature! A few things I noticed though:
- Is it really intended that the search bar goes missing? I would at least expect the search icon like in the mobile view.
- Is it possible to align the tabs to the left? Having them centered feels a bit weird to me.
- The disabled-/enabled-states of the "save"-button in the text-editor hardly differ from each other, especially in light mode. That makes it hard to tell if the button is clickable or not.
Strongly agree with @JammingBen's opinion on all three points... the first and second are blockers for me on the course of getting this merged. |
Pasting my feedback from ownCloud talk:
|
Thanks for porting your feedback ;) Responses in order
|
Thanks for the feedback. In order:
|
relates to 31e2ad6 |
I understand, but each app has it's own place for the download button. Having a consistent place (even if duplicated) for users to search for it would be an improvement, in my view. |
@pascalwengerter After talking with @tbsbdr : it's okay to drop the search bar, at least for now. Alignment to the left and the button color are still valid points. Also, we think it might be better to make the tab more decent looking. Goal: it should be clear that actions belong to the tab. Let's do it like that: Just ping me if I can help you with some of these points as we want to get that merged soon.
I agree, although I'd suggest doing that in a follow-up PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buttons look very nice now 👍 The tabs are still aligned in the center and missing their border though (see #8447 (comment)).
b16a742
to
69930fd
Compare
Alignment was a major PITA and yeah, I did mess up the ODS color token/theme situation but should be fixed now ;) |
075bbb8
to
69930fd
Compare
method: '', | ||
formParameters: {} | ||
resource: <Resource>null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to cast null
@@ -33,7 +34,7 @@ export default defineComponent({ | |||
loading: true, | |||
loadingError: false, | |||
url: '', | |||
resource: null | |||
resource: <Resource>null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
69930fd
to
695d672
Compare
e42b173
to
43aa0d6
Compare
…o pass appearance&variation for all actions
The drone-ci failure is due to the new version availability of the notifications app. We can either upgrade the app with |
I think add line Lines 2269 to 2276 in 15e09d8
"cd %s || exit" % dir["server"],
+ "php occ upgrade",
"php occ a:e notifications", |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 🥳
Description
Details (esp. design) tbd, just wanted to make this take into consideration the existing apps (and try to limit the codechanges needed)
Related Issue