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

link from app-navigation-settings to personal settings #621

Merged
merged 1 commit into from
Jul 24, 2021

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Jul 22, 2021

Fix #274

Before After
image image

Signed-off-by: szaimen szaimen@e.mail.de

@szaimen szaimen added this to the Nextcloud 23 milestone Jul 22, 2021
@szaimen szaimen force-pushed the enh/noid/personal-settings-link branch 19 times, most recently from ee7da82 to 834c3fc Compare July 22, 2021 17:16
@szaimen szaimen marked this pull request as ready for review July 22, 2021 17:26
@szaimen szaimen requested review from artonge and CarlSchwan July 22, 2021 17:40
templates/stream.app.navigation.php Outdated Show resolved Hide resolved
css/style.scss Outdated

a {
display: block;
padding: 10px 0px 10px 27px;
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker for this PR but I see a lot of these magic numbers everywhere in Nextcloud codebase. Is there a plan to unify them a bit? Personally, I like the way this is done in bootstrap/tailwind with css utilities like p-1, p-2, p-3, p-4, p-5

Copy link
Contributor Author

@szaimen szaimen Jul 22, 2021

Choose a reason for hiding this comment

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

hm... cannot answer that. I just put them in place so that the link is good shaped for this specific situation...

Copy link
Member

Choose a reason for hiding this comment

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

Is there a plan to unify them a bit? Personally, I like the way this is done in bootstrap/tailwind with css utilities like p-1, p-2, p-3, p-4, p-5

Would require further discussions with @nextcloud/server team :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did align it now to padding: 10px 0px 10px 25px which seems like the perfect values here...

Copy link
Member

Choose a reason for hiding this comment

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

The general idea was more to move to shared vue components rather than implementing more global styles. For magic values we should slowly move more towards using css variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it okay to merge this then or is there something I should change in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

@juliushaertl ok :) yeah shared Vue components make sense :)

@szaimen: Ship it 🚀

@szaimen szaimen force-pushed the enh/noid/personal-settings-link branch from 834c3fc to 7711080 Compare July 22, 2021 18:09
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Would it be possible to align the link text with the RSS text above? Just slightly off in the screenshot at the moment :)

@szaimen szaimen force-pushed the enh/noid/personal-settings-link branch from 7711080 to ff78e91 Compare July 22, 2021 19:29
@szaimen
Copy link
Contributor Author

szaimen commented Jul 22, 2021

Would it be possible to align the link text with the RSS text above? Just slightly off in the screenshot at the moment :)

Oh wow, how in the world did you manage to see that? 😱

It is fixed now :)

@@ -53,6 +53,11 @@
<input id="feed-link" class="feed-link" type="text" readonly="readonly" value="<?php p($_['rssLink']); ?>" />
<a class="icon-clippy" data-clipboard-target="#rssurl input"></a>
</span>
<div id="activity-personal-settings-link">
<a href="<?php p($_['personalSettingsLink']); ?>">
<span class="no-icon"><?php p($l->t('Personal Activity Settings') . ' ↗'); ?></span>
Copy link
Member

Choose a reason for hiding this comment

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

I think we only use the for external links, but pinging @jancborchardt to clarify on that.

Copy link
Member

Choose a reason for hiding this comment

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

We use either the icon-external or ↗ for external links. I think it's better not to use any icon here or maybe the activity app icon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. This is is how it looks without :
image

Signed-off-by: szaimen <szaimen@e.mail.de>
@szaimen szaimen force-pushed the enh/noid/personal-settings-link branch from ff78e91 to f124ff6 Compare July 24, 2021 09:55
@szaimen szaimen merged commit dfe1ccf into master Jul 24, 2021
@delete-merged-branch delete-merged-branch bot deleted the enh/noid/personal-settings-link branch July 24, 2021 10:06
@szaimen
Copy link
Contributor Author

szaimen commented Jul 24, 2021

/backport to stable22

@szaimen
Copy link
Contributor Author

szaimen commented Jul 24, 2021

/backport to stable21

@szaimen
Copy link
Contributor Author

szaimen commented Jul 24, 2021

/backport to stable20

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.

Request: Allow users to configure Stream settings from within Activity App
6 participants