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

Add warning to pin to frontpage #4999

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

emillub
Copy link

@emillub emillub commented Sep 25, 2024

Description

Add warning under "fest til forsiden" to make i clear you need permission to pin.
Applies to article- and event-editing.

Result

If you've made visual changes, please check the boxes below and include images showing the changes. Descriptions are appreciated.

  • Changes look good on both light and dark theme.
  • Changes look good with different viewports (mobile, tablet, etc.).
  • Changes look good with slower Internet connections.
Description Before After
... image image

Testing

  • I have thoroughly tested my changes.

Please describe what and how the changes have been tested, and provide instructions to reproduce if necessary.


Resolves: ABA-1077

Copy link

linear bot commented Sep 25, 2024

Copy link

vercel bot commented Sep 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
lego-bricks-storybook ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 5:50pm

@github-actions github-actions bot added the review-needed Pull requests that need review label Sep 25, 2024
Copy link
Contributor

@christiangryt christiangryt left a comment

Choose a reason for hiding this comment

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

LGTM!

@emillub emillub force-pushed the add-warning-to-pin-to-front-page branch from 498eed2 to 15af74b Compare September 25, 2024 20:20
@ch0rizo
Copy link
Contributor

ch0rizo commented Sep 25, 2024

LGMT!

Copy link
Member

@norbye norbye left a comment

Choose a reason for hiding this comment

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

LGTM! Talked to Falk and it doesn't seem like there will be a document for it - so we can just merge this now and rather update it if anything should change

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Nice!😍 Some nitpicks

@@ -179,6 +180,15 @@ const ArticleEditor = () => {
type="checkbox"
component={CheckBox.Field}
/>
{values.pinned && (
<Card style={{ marginTop: 0 }} severity="warning">
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we don't want any inline styling

{values.pinned && (
<Card style={{ marginTop: 0 }} severity="warning">
<span>
OBS! Du må ha godkjenning fra ledelsen for å feste til
Copy link
Member

Choose a reason for hiding this comment

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

Could the "obs" be in a Card.Header so that it matches other cards better?😁

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Comment on lines 70 to 73
<Card style={{ marginTop: 0 }} severity="warning">
<span>
OBS! Du må ha godkjenning fra ledelsen for å feste til forsiden.
</span>
Copy link
Member

Choose a reason for hiding this comment

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

See other comments

Copy link
Author

Choose a reason for hiding this comment

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

Juni and I concluded that the syling wasn't necessary so we removed it:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants