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.

Sign up for GitHub

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

Jump to bottom

Support section header actions #9161

Merged
merged 37 commits into from
Nov 20, 2023
Merged

Support section header actions #9161

merged 37 commits into from
Nov 20, 2023

Conversation

wychoong
Copy link
Contributor

@wychoong wychoong commented Oct 18, 2023 •
edited

  • Changes have been thoroughly tested to not break existing functionality.
  • New functionality has been documented or existing documentation has been updated to reflect changes.
  • Visual changes are explained in the PR description using a screenshot/recording of before and after.

infolist section
image

form section
image

wrapped
image

zepfietje
zepfietje requested changes Oct 18, 2023
View reviewed changes
Copy link
Member

@zepfietje zepfietje left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for the PR, @wychoong!

Could you have a look at my comment?
Also, @danharrin, don't we need some caching for the actions?

Additionally, maybe the API should be renamed to headerActions?

packages/support/resources/views/components/section/index.blade.php Outdated
@@ -124,6 +130,12 @@
</div>
@endif

@if ($hasActions)
<div class="flex flex-1 justify-end">
Copy link
Member

Choose a reason for hiding this comment

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

We can probably remove the wrapper and directly add a class like ms-auto to the actions component used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats better, done!

Copy link
Contributor Author

@wychoong wychoong Oct 18, 2023 •
edited

Choose a reason for hiding this comment

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

the only difference I see is that now heading and action container is not taking half as no more flex-1
image

but it looks better as it doesnt wrap when larger than md
or do you think need to responsive based on viewport size

Copy link
Member

Choose a reason for hiding this comment

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

Hmm but the heading wrapper has flex-1? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup it does

@zepfietje zepfietje changed the title Add actions to infolist and form section heading Support section actions Oct 18, 2023
@zepfietje zepfietje marked this pull request as draft October 18, 2023 10:25
@zepfietje zepfietje added the enhancement New feature or request label Oct 18, 2023
@zepfietje zepfietje added this to the v3 milestone Oct 18, 2023
@wychoong
Copy link
Contributor Author

Also, @danharrin, don't we need some caching for the actions?

getActions is cached

Additionally, maybe the API should be renamed to headerActions?

probably an alias to actions? or you thinking to support footerActions?

@zepfietje
Copy link
Member

Yeah potentially there will be footer actions in the future, so I'd prefer to have specific APIs.

@wychoong
Copy link
Contributor Author

Yeah potentially there will be footer actions in the future, so I'd prefer to have specific APIs.

I see. I think now only remain the api. I’ll leave it up to maintainers to decide for changes

@zepfietje zepfietje marked this pull request as ready for review October 18, 2023 12:27
@zepfietje
Copy link
Member

Requesting a review from @danharrin for the API.
Note that I still want to review/rework the implementation in the view though.

@akunbeben
Copy link
Contributor

Can't wait to have this feature 🎉

@zepfietje
Copy link
Member

@wychoong, could you rework this to be headerActions?

@zepfietje zepfietje marked this pull request as draft November 2, 2023 21:51
@wychoong wychoong marked this pull request as ready for review November 7, 2023 08:22
@zepfietje zepfietje marked this pull request as ready for review November 8, 2023 12:32
@zepfietje zepfietje self-assigned this Nov 9, 2023
@danharrin danharrin self-assigned this Nov 20, 2023
@danharrin danharrin removed their assignment Nov 20, 2023
@zepfietje zepfietje changed the title Support section actions Support section header actions Nov 20, 2023
@zepfietje zepfietje merged commit a268e5f into filamentphp:3.1 Nov 20, 2023
4 checks passed
@wychoong wychoong deleted the add-action-to-section-heading branch November 30, 2023 09:50
This was referenced Mar 1, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danharrin danharrin danharrin approved these changes

@zepfietje zepfietje Awaiting requested review from zepfietje

Assignees

@zepfietje zepfietje

Labels
enhancement New feature or request
Projects
Roadmap
Archived in project
Milestone
v3.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wychoong @zepfietje @akunbeben @danharrin